Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-44

Remove build system dependencies from tests

    Details

    • Type: RFC
    • Status: Implemented
    • Resolution: Done
    • Component/s: DM
    • Labels:
      None
    • Location:
      this ticket

      Description

      The discussion in DM-2527 demonstrated that test code uses EUPS to determine where things are located. This has two problems:

      1. Calling EUPS in a loop resulted in slow downs of the tests as the EUPS database was repeatedly read.
      2. This makes it impossible to run the tests standalone and burns-in a particular packaging implementation that is not relevant for the tests.

      This RFC proposes that EUPS be removed from tests and an alternative scheme be used for locating supporting infrastructure. Some tests still need to locate other packages (in particular afwdata) so any replacement scheme must support some form of package location.

      Some options for locating afwdata:

      1. Provide a test utility function for locating a package which can use an environment variable to find afwdata.
      2. Add afwdata as a git submodule in the test directory. This could result in multiple checkouts of the repository.

      This RFC invites further discussion on whether EUPS should be removed from the tests. Comments on replacement functionality are welcomed to demonstrate feasibility.

        Attachments

          Issue Links

            Activity

            Hide
            frossie Frossie Economou added a comment -

            Dumb question, but do all our repos depend on the butler (and I don't mean in actual fact, I mean for architectural reasons). If not, surely creating a dependence on the butler replaces one problem with another?

            Show
            frossie Frossie Economou added a comment - Dumb question, but do all our repos depend on the butler (and I don't mean in actual fact, I mean for architectural reasons). If not, surely creating a dependence on the butler replaces one problem with another?
            Hide
            rhl Robert Lupton added a comment -

            For this task I think an environment variable is fine, and in fact that's all eups.getProductDir() does under the covers. I think that tho means that the eups lookup could be made fast in this case but I agree that it's not a good idea; for now we could use eups to set the environment variable (a task it does well).

            We'd wrap this in a function in e.g. lsst.utils so that we have the freedom to change implementations.

            I don't like the multiple git checkout version. One reason to move test data to afwdata was to stop this data duplication.

            The use of eups in meas_astrom is more of a problem than this (we use it to manage multiple data sets simultaneously). There can be one "current" afwdata, but there's no preferred astrometric catalogue. I suspect this will need a different design.

            Show
            rhl Robert Lupton added a comment - For this task I think an environment variable is fine, and in fact that's all eups.getProductDir() does under the covers. I think that tho means that the eups lookup could be made fast in this case but I agree that it's not a good idea; for now we could use eups to set the environment variable (a task it does well). We'd wrap this in a function in e.g. lsst.utils so that we have the freedom to change implementations. I don't like the multiple git checkout version. One reason to move test data to afwdata was to stop this data duplication. The use of eups in meas_astrom is more of a problem than this (we use it to manage multiple data sets simultaneously). There can be one "current" afwdata, but there's no preferred astrometric catalogue. I suspect this will need a different design.
            Hide
            tjenness Tim Jenness added a comment -

            There were no objections to the concept of removing eups from the stack runtime and test python code. Implementation decisions will be handled in an explicit ticket.

            Show
            tjenness Tim Jenness added a comment - There were no objections to the concept of removing eups from the stack runtime and test python code. Implementation decisions will be handled in an explicit ticket.
            Hide
            rowen Russell Owen added a comment - - edited

            There are three relevant tickets:

            • DM-2635 and DM-2636 deal with lsst::utils::eups::packageDir and eups.packageDir.
            • DM-2527 deals with CameraMapper.getEupsPackageName

            Those are the only instances of using eups in our code. If any others are found, please mention them here.

            Show
            rowen Russell Owen added a comment - - edited There are three relevant tickets: DM-2635 and DM-2636 deal with lsst::utils::eups::packageDir and eups.packageDir. DM-2527 deals with CameraMapper.getEupsPackageName Those are the only instances of using eups in our code. If any others are found, please mention them here.
            Hide
            tjenness Tim Jenness added a comment -

            Joshua Hoblitt has found some cases of SCons using eups when it doesn't really need to. eups itself is directly imported by code in datarel, daf_butlerUtils, obs_lsstSim, obs_sdss, obs_test, pipe_base and sconsUtils. More directly relevant to this ticket is that 44 tests (at least) import eups and a random pick of three determined (from pylint perspective) that two of them didn't require the import anyhow (presumably test boilerplate). We need to clean out the cruft so that we know what really needs eups and what is just pretending to need eups. It looks like the ones that require eups are using eups.productDir. I assume these should (mostly) be replaced with calls to the utils function. We missed them all the last time we discussed this.

            Show
            tjenness Tim Jenness added a comment - Joshua Hoblitt has found some cases of SCons using eups when it doesn't really need to. eups itself is directly imported by code in datarel, daf_butlerUtils, obs_lsstSim, obs_sdss, obs_test, pipe_base and sconsUtils. More directly relevant to this ticket is that 44 tests (at least) import eups and a random pick of three determined (from pylint perspective) that two of them didn't require the import anyhow (presumably test boilerplate). We need to clean out the cruft so that we know what really needs eups and what is just pretending to need eups. It looks like the ones that require eups are using eups.productDir . I assume these should (mostly) be replaced with calls to the utils function. We missed them all the last time we discussed this.

              People

              • Assignee:
                tjenness Tim Jenness
                Reporter:
                tjenness Tim Jenness
                Watchers:
                Frossie Economou, Jim Bosch, John Swinbank, Mario Juric, Robert Lupton, Russell Owen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                7 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel