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

Modify cat tests to support py.test

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: cat

      Description

      This ticket is for the work of migrating the cat tests such that they run with the py.test test runner.

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            Adding Andy Salnikov as we may be able to just remove the cat package (and if not, the tests need a lot of work).

            Show
            Parejkoj John Parejko added a comment - Adding Andy Salnikov as we may be able to just remove the cat package (and if not, the tests need a lot of work).
            Hide
            tjenness Tim Jenness added a comment -

            It looks like ctrl_orca, ctrl_provenance, and dax_imgserv are the packages that depend on cat. If we can get rid of cat completely from these packages then that would be even better than going through the pain of updating this package's tests. Steve Pietrowicz do you have any thought on whether cat is really needed for the ctrl_ packages?

            Show
            tjenness Tim Jenness added a comment - It looks like ctrl_orca , ctrl_provenance , and dax_imgserv are the packages that depend on cat . If we can get rid of cat completely from these packages then that would be even better than going through the pain of updating this package's tests. Steve Pietrowicz do you have any thought on whether cat is really needed for the ctrl_ packages?
            Hide
            salnikov Andy Salnikov added a comment - - edited

            It looks like dax_imgserv does not really need cat dependency, there is one script which imports lsst.cat.dbSetup but never uses that. We should cleanup that script and update table file to remove cat dependency. Same is probably true ctrl_provenance, cat is in a table file but it does not really use anything from cat. ctrl_orca has few imports from cat which are used, so this needs to be looked at and probably replaced with db use.

            About cat itself - out of four files in test/ directory only one (tests/timeFuncs.py) is a unit test, all others are regular scripts which are implementing tests that cannot be run as unit tests (they need user input). I can look at timeFuncs.py and see if it is possible to migrate it to pytest. I suppose we can leave three other scripts alone.

            Show
            salnikov Andy Salnikov added a comment - - edited It looks like dax_imgserv does not really need cat dependency, there is one script which imports lsst.cat.dbSetup but never uses that. We should cleanup that script and update table file to remove cat dependency. Same is probably true ctrl_provenance , cat is in a table file but it does not really use anything from cat . ctrl_orca has few imports from cat which are used, so this needs to be looked at and probably replaced with db use. About cat itself - out of four files in test/ directory only one ( tests/timeFuncs.py ) is a unit test, all others are regular scripts which are implementing tests that cannot be run as unit tests (they need user input). I can look at timeFuncs.py and see if it is possible to migrate it to pytest . I suppose we can leave three other scripts alone.
            Hide
            spietrowicz Steve Pietrowicz added a comment -

            For ctrl_orca, the files that are used are under $CAT_DIR/sql (setup_storeFunctions.sql, setup_perRunTable*.sql, lsstSchema4mysql*.sql). Those are set up in the lsst10 database, but I'm not sure how (or even if) the tables are being used by the current pipelines. I believe the intent at the time was to have orchestration set those up blindly, and cat would change as the pipelines saw fit. That way orchestration didn't have to be rev-ed every time that cat SQL files needed updating, or worse, that we'd have to have people download ctrl_orca and mess around with its innards to get at a couple of SQL files that they wanted to modify. All those SQL files could probably move to new package (cat_sql?), if we wanted to retain that functionality. I don't believe ctrl_orca uses anything else in that package.

            As for ctrl_orca's use of ctrl_provenance... Way back, when the original .paf files used by orchestration went away, I removed the calls to methods in ctrl_orca that end up calling ctrl_provenance because a new provenance method was going to be on the way. All of the code remained, so I would be able to add that functionality back in at some point.

            I believe that ctrl_provenance itself doesn't make direct references to the SQL files in cat, but they are logically bound. In other words, the provenance recorder expects that the "prv_Run" table be in place before the time the ProvenanceRecorder is called. My memory is a bit shaky about this because I didn't write that code. There might be direct dependencies that I'm not aware of. Again, ctrl_orca doesn't use ctrl_provenance at the moment, but retains the dependency for the time in the future when the new ctrl_provenance arrives.

            Show
            spietrowicz Steve Pietrowicz added a comment - For ctrl_orca, the files that are used are under $CAT_DIR/sql (setup_storeFunctions.sql, setup_perRunTable*.sql, lsstSchema4mysql*.sql). Those are set up in the lsst10 database, but I'm not sure how (or even if) the tables are being used by the current pipelines. I believe the intent at the time was to have orchestration set those up blindly, and cat would change as the pipelines saw fit. That way orchestration didn't have to be rev-ed every time that cat SQL files needed updating, or worse, that we'd have to have people download ctrl_orca and mess around with its innards to get at a couple of SQL files that they wanted to modify. All those SQL files could probably move to new package (cat_sql?), if we wanted to retain that functionality. I don't believe ctrl_orca uses anything else in that package. As for ctrl_orca's use of ctrl_provenance... Way back, when the original .paf files used by orchestration went away, I removed the calls to methods in ctrl_orca that end up calling ctrl_provenance because a new provenance method was going to be on the way. All of the code remained, so I would be able to add that functionality back in at some point. I believe that ctrl_provenance itself doesn't make direct references to the SQL files in cat, but they are logically bound. In other words, the provenance recorder expects that the "prv_Run" table be in place before the time the ProvenanceRecorder is called. My memory is a bit shaky about this because I didn't write that code. There might be direct dependencies that I'm not aware of. Again, ctrl_orca doesn't use ctrl_provenance at the moment, but retains the dependency for the time in the future when the new ctrl_provenance arrives.
            Hide
            salnikov Andy Salnikov added a comment -

            Steve Pietrowicz, ctrl_orca has few imports of lsst.cat: https://github.com/lsst/ctrl_orca/search?utf8=%E2%9C%93&q=lsst.cat

            I have very little understanding of what python modules is cat package do, so I can't tell right away if they should or should not be replaced with something else.

            Show
            salnikov Andy Salnikov added a comment - Steve Pietrowicz , ctrl_orca has few imports of lsst.cat : https://github.com/lsst/ctrl_orca/search?utf8=%E2%9C%93&q=lsst.cat I have very little understanding of what python modules is cat package do, so I can't tell right away if they should or should not be replaced with something else.
            Hide
            salnikov Andy Salnikov added a comment -

            John, could you review it: https://github.com/lsst/cat/pull/6
            There is only one unit test in the package and all tests are being skipped (because of some missing config file which I do not know how to setup) so it actually succeeds without running any tests. It may need more work later but py.test migration part is probably OK.

            Show
            salnikov Andy Salnikov added a comment - John, could you review it: https://github.com/lsst/cat/pull/6 There is only one unit test in the package and all tests are being skipped (because of some missing config file which I do not know how to setup) so it actually succeeds without running any tests. It may need more work later but py.test migration part is probably OK.
            Hide
            Parejkoj John Parejko added a comment -

            Your py.test migration is good.

            We should definitely see if we can just delete cat. Can you make a ticket for that and have someone dig into it? Dead and dying dependencies should go!

            Show
            Parejkoj John Parejko added a comment - Your py.test migration is good. We should definitely see if we can just delete cat. Can you make a ticket for that and have someone dig into it? Dead and dying dependencies should go!
            Hide
            salnikov Andy Salnikov added a comment -

            Thanks, John! I made new ticket (DM-7237) to see if we can do something with a dead cat Closing this ticket.

            Show
            salnikov Andy Salnikov added a comment - Thanks, John! I made new ticket ( DM-7237 ) to see if we can do something with a dead cat Closing this ticket.

              People

              • Assignee:
                salnikov Andy Salnikov
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                John Parejko
                Watchers:
                Andy Salnikov, Brian Van Klaveren, John Parejko, Steve Pietrowicz, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel