Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-674

fix handling of nested control objects

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: pex_config
    • Labels:

      Description

      Work on the HSC side has revealed some problems with nested control objects being wrapped into config objects. This is a pull request for those changes (along with writing a unit test for some of them).

      Some (but not all of these changes) are part of Trac ticket #3163 (https://dev.lsstcorp.org/trac/ticket/3163), which I'll now close as a duplicate.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Ready for review on u/jbosch/DM-674 of pex_config:

            pex_config:u/jbosch/DM-674 % git diff --stat master...u/jbosch/DM-674
             python/lsst/pex/config/wrap.py |   36 +++++++++++++++++++-----------------
             tests/testLib.i                |   11 +++++++----
             tests/wrap.py                  |    8 +++++---
             3 files changed, 31 insertions(+), 24 deletions(-)

            The changeset is pretty small, but it's got some deep pex_config internals (more precisely, it adds pre-existing pex_config internals that improve history readability to the control-object-wrapping code).

            Show
            jbosch Jim Bosch added a comment - Ready for review on u/jbosch/ DM-674 of pex_config: pex_config:u/jbosch/DM-674 % git diff --stat master...u/jbosch/DM-674 python/lsst/pex/config/wrap.py | 36 +++++++++++++++++++----------------- tests/testLib.i | 11 +++++++---- tests/wrap.py | 8 +++++--- 3 files changed, 31 insertions(+), 24 deletions(-) The changeset is pretty small, but it's got some deep pex_config internals (more precisely, it adds pre-existing pex_config internals that improve history readability to the control-object-wrapping code).
            Hide
            rowen Russell Owen added a comment -

            Overall this looks like good.

            It's a pity to add internal arguments to readControl, but they match Config.update and avoid iterating over fields in two places, so it certainly seems justified. (If it also improves functionality, so much the better. I confess I couldn't see such a difference.)

            wrap.py:

            • You added support for int64_t. Why just that one boost int type? In particular, I hope this doesn't assume the type of python int and C++ int. I suggest adding a comment to the code explaining.
            • In _containerRegex I see support for std::vector and some kind of list. Should it also support std::array (or std::tr1::array) for when we switch to C++ 11?

            testLib.i and wrap.py:

            • I suggest a test for boost::int64_t
            Show
            rowen Russell Owen added a comment - Overall this looks like good. It's a pity to add internal arguments to readControl, but they match Config.update and avoid iterating over fields in two places, so it certainly seems justified. (If it also improves functionality, so much the better. I confess I couldn't see such a difference.) wrap.py: You added support for int64_t. Why just that one boost int type? In particular, I hope this doesn't assume the type of python int and C++ int. I suggest adding a comment to the code explaining. In _containerRegex I see support for std::vector and some kind of list. Should it also support std::array (or std::tr1::array) for when we switch to C++ 11? testLib.i and wrap.py: I suggest a test for boost::int64_t
            Hide
            jbosch Jim Bosch added a comment -

            It's a pity to add internal arguments to readControl, but they match Config.update and avoid iterating over fields in two places, so it certainly seems justified. (If it also improves functionality, so much the better. I confess I couldn't see such a difference.)

            It's subtle: the real bug that kicked all of this off was that previously, when an outer control object changed the value of an inner control object in its constructor (the moral equivalent of changing a sub-config value in the outer config's setDefaults()), this was not propagated to the defaults in the generated config object. Using the internal arguments and delegating to update() allowed me to do some cleanup while fixing that bug.

            • You added support for int64_t. Why just that one boost int type? In particular, I hope this doesn't assume the type of python int and C++ int. I suggest adding a comment to the code explaining.

            It only assumes that Python's int is large enough to hold 64-bit values, which I believe has been true even on 32-bit platforms since Python 2.4. I've added a comment explaining that we're not looking for binary under-the-hood compatibility here, just the ability to round-trip through the usual Swig converters.

            • In _containerRegex I see support for std::vector and some kind of list. Should it also support std::array (or std::tr1::array) for when we switch to C++ 11?

            Oh, probably, but it'll be trickier since it has an extra template parameter too, so I don't plan to add until we have a use case for it. In any case, I've given up holding my breath on C++11 in the stack for fear of asphyxiation

            • I suggest a test for boost::int64_t

            Done.

            Show
            jbosch Jim Bosch added a comment - It's a pity to add internal arguments to readControl, but they match Config.update and avoid iterating over fields in two places, so it certainly seems justified. (If it also improves functionality, so much the better. I confess I couldn't see such a difference.) It's subtle: the real bug that kicked all of this off was that previously, when an outer control object changed the value of an inner control object in its constructor (the moral equivalent of changing a sub-config value in the outer config's setDefaults()), this was not propagated to the defaults in the generated config object. Using the internal arguments and delegating to update() allowed me to do some cleanup while fixing that bug. You added support for int64_t. Why just that one boost int type? In particular, I hope this doesn't assume the type of python int and C++ int. I suggest adding a comment to the code explaining. It only assumes that Python's int is large enough to hold 64-bit values, which I believe has been true even on 32-bit platforms since Python 2.4. I've added a comment explaining that we're not looking for binary under-the-hood compatibility here, just the ability to round-trip through the usual Swig converters. In _containerRegex I see support for std::vector and some kind of list. Should it also support std::array (or std::tr1::array) for when we switch to C++ 11? Oh, probably, but it'll be trickier since it has an extra template parameter too, so I don't plan to add until we have a use case for it. In any case, I've given up holding my breath on C++11 in the stack for fear of asphyxiation I suggest a test for boost::int64_t Done.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Russell Owen
              Watchers:
              Jim Bosch, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.