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

Port ctrl_provenance to Python 3 and update unit tests for pytest

    XMLWordPrintable

    Details

      Attachments

        Issue Links

          Activity

          No builds found.
          rowen Russell Owen created issue -
          rowen Russell Owen made changes -
          Field Original Value New Value
          Status To Do [ 10001 ] In Progress [ 3 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          rowen Russell Owen made changes -
          Comment [ Neither unit test ever tested memory leakage before. I added the memory tests to see what would happen and found that {{testProvenanceRecorder.py}} passed, but {{testProvenanceSetup.py}} failed. Is this likely to indicate an actual bug? I'll disable the test in {{testProvenanceSetup.py}} for now, but I'd like to know. ]
          rowen Russell Owen made changes -
          Hide
          rowen Russell Owen added a comment - - edited

          Jenkins py2 passes. Need the cat package to be updated before testing py3

          Show
          rowen Russell Owen added a comment - - edited Jenkins py2 passes. Need the cat package to be updated before testing py3
          rowen Russell Owen made changes -
          Link This issue is blocked by DM-7260 [ DM-7260 ]
          rowen Russell Owen made changes -
          Watchers Russell Owen, Steve Pietrowicz, Tim Jenness [ Russell Owen, Steve Pietrowicz, Tim Jenness ] Andy Salnikov, Russell Owen, Steve Pietrowicz, Tim Jenness [ Andy Salnikov, Russell Owen, Steve Pietrowicz, Tim Jenness ]
          Hide
          tjenness Tim Jenness added a comment -

          We may want to consider adding the tests but skipping them so that we get a reminder that they are disabled. We can do that by adding a setUp function to the test class and have it immediately raise a skip exception.

          Show
          tjenness Tim Jenness added a comment - We may want to consider adding the tests but skipping them so that we get a reminder that they are disabled. We can do that by adding a setUp function to the test class and have it immediately raise a skip exception.
          Hide
          rowen Russell Owen added a comment -

          There was no real memory leak, just missing content in the tearDown method. All is well (at least in Python 2)

          Show
          rowen Russell Owen added a comment - There was no real memory leak, just missing content in the tearDown method. All is well (at least in Python 2)
          Hide
          tjenness Tim Jenness added a comment -

          Builds for me (now that cat has been merged).

          Show
          tjenness Tim Jenness added a comment - Builds for me (now that cat has been merged).
          jmatt J Matt Peterson [X] (Inactive) made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          Hide
          rowen Russell Owen added a comment -

          Steve, do you have time to look at this?

          Show
          rowen Russell Owen added a comment - Steve, do you have time to look at this?
          rowen Russell Owen made changes -
          Status In Progress [ 3 ] In Review [ 10004 ]
          Reviewers Steve Pietrowicz [ spietrowicz ]
          spietrowicz Steve Pietrowicz made changes -
          Comment [ I can look at this. Question though: In the "Component/s" above, this is listed as ctrl_events. Is that really meant to be ctrl_provenance? ]
          swinbank John Swinbank made changes -
          Component/s ctrl_provenance [ 13621 ]
          Component/s ctrl_events [ 10718 ]
          Hide
          spietrowicz Steve Pietrowicz added a comment -

          This all looks fine. The only thing I noticed was that the tests inherited from unittest.TestCase instead of lsst.utils.tests.TestCase. I'm not sure that it completely required, but that's what they did in the SQR-12.lsst.io porting guide.

          Show
          spietrowicz Steve Pietrowicz added a comment - This all looks fine. The only thing I noticed was that the tests inherited from unittest.TestCase instead of lsst.utils.tests.TestCase. I'm not sure that it completely required, but that's what they did in the SQR-12.lsst.io porting guide.
          spietrowicz Steve Pietrowicz made changes -
          Status In Review [ 10004 ] Reviewed [ 10101 ]
          nlust Nate Lust made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          Hide
          tjenness Tim Jenness added a comment -

          You only need to inherit from utils TestCase if you are using asserts from that test case. So it doesn't really matter.

          I did have a quick look at the PR and I have a concern over the iterator in daf_persistence. I can't actually test the DbStorage_1.py usage of next because the tests skip for me even if I'm on lsst-dev. I need to know how the end of the iterator is triggered from C++. It might be that I have to intercept some other condition and translate it to StopIteration. Steve Pietrowicz do you know what machine I have to be on for the DbStorage_1.py test to run or is the server not actually active any more?

          Show
          tjenness Tim Jenness added a comment - You only need to inherit from utils TestCase if you are using asserts from that test case. So it doesn't really matter. I did have a quick look at the PR and I have a concern over the iterator in daf_persistence . I can't actually test the DbStorage_1.py usage of next because the tests skip for me even if I'm on lsst-dev . I need to know how the end of the iterator is triggered from C++. It might be that I have to intercept some other condition and translate it to StopIteration . Steve Pietrowicz do you know what machine I have to be on for the DbStorage_1.py test to run or is the server not actually active any more?
          tjenness Tim Jenness made changes -
          Watchers Andy Salnikov, Russell Owen, Steve Pietrowicz, Tim Jenness [ Andy Salnikov, Russell Owen, Steve Pietrowicz, Tim Jenness ] Andy Salnikov, Nate Pease, Russell Owen, Steve Pietrowicz, Tim Jenness [ Andy Salnikov, Nate Pease, Russell Owen, Steve Pietrowicz, Tim Jenness ]
          Hide
          spietrowicz Steve Pietrowicz added a comment -

          I believe what's happening in DbStorage_1.py is that it checks the $HOME/.lsst/db-auth.paf file for auth info for lsst10.ncsa.uiuc.edu (this really should be lsst10.ncsa.illinois.edu) at port 3306. If you do not have that entry in that file, the test will be skipped.

          Show
          spietrowicz Steve Pietrowicz added a comment - I believe what's happening in DbStorage_1.py is that it checks the $HOME/.lsst/db-auth.paf file for auth info for lsst10.ncsa.uiuc.edu (this really should be lsst10.ncsa.illinois.edu) at port 3306. If you do not have that entry in that file, the test will be skipped.
          Hide
          tjenness Tim Jenness added a comment -

          Thank you. That explains why I had a memory of the tests running okay on my laptop and I was confused on my iMac and lsst-dev. I'll take a quick look at daf_persistence. Russell Owen can we hold off merging until I've worked out whether we need to be catching StopIteration in the code treating it as a proper Python 3 iterator.

          Show
          tjenness Tim Jenness added a comment - Thank you. That explains why I had a memory of the tests running okay on my laptop and I was confused on my iMac and lsst-dev. I'll take a quick look at daf_persistence. Russell Owen can we hold off merging until I've worked out whether we need to be catching StopIteration in the code treating it as a proper Python 3 iterator.
          Hide
          tjenness Tim Jenness added a comment -

          Ok. It looks like daf_persistence::next is not actually an iterator and should not be confused as one. It's job is just to move a pointer to the next result and not to return the result so it should never be used like a Python iterator. The name is a major source of confusion when switching from C++ to Python3. It looks like I should revert DM-7226 and ctrl_provenance should go back to using the next() method and not assuming it's at all related to the Python next.

          Show
          tjenness Tim Jenness added a comment - Ok. It looks like daf_persistence::next is not actually an iterator and should not be confused as one. It's job is just to move a pointer to the next result and not to return the result so it should never be used like a Python iterator. The name is a major source of confusion when switching from C++ to Python3. It looks like I should revert DM-7226 and ctrl_provenance should go back to using the next() method and not assuming it's at all related to the Python next .
          Hide
          tjenness Tim Jenness added a comment -

          Ideally we'd deprecate next and rename it to nextResult in daf_persistence.

          Show
          tjenness Tim Jenness added a comment - Ideally we'd deprecate next and rename it to nextResult in daf_persistence .
          rowen Russell Owen made changes -
          Link This issue relates to DM-7226 [ DM-7226 ]
          Hide
          rowen Russell Owen added a comment -

          As per Tim Jenness request I changed next(self._globalDb) to self._globalDb.next() in dc3.py. I also fixed flake8 warnings and errors and standardized imports. I'm ready to merge after a Jenkins run.

          Show
          rowen Russell Owen added a comment - As per Tim Jenness request I changed next(self._globalDb) to self._globalDb.next() in dc3.py . I also fixed flake8 warnings and errors and standardized imports. I'm ready to merge after a Jenkins run.
          rowen Russell Owen made changes -
          Link This issue relates to DM-7323 [ DM-7323 ]
          Hide
          rowen Russell Owen added a comment -

          I also filed DM-7323 as per Tim Jenness suggestion about renaming DbStorage.next

          Show
          rowen Russell Owen added a comment - I also filed DM-7323 as per Tim Jenness suggestion about renaming DbStorage.next
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          Hide
          rowen Russell Owen added a comment - - edited

          Jenkins Python 2 passed and Steve Pietrowicz enhanced the documentation for ProvenanceSetup.addWorkflowRecordCmd. Merged to master.

          Show
          rowen Russell Owen added a comment - - edited Jenkins Python 2 passed and Steve Pietrowicz enhanced the documentation for ProvenanceSetup.addWorkflowRecordCmd . Merged to master.
          rowen Russell Owen made changes -
          Resolution Done [ 10000 ]
          Status Reviewed [ 10101 ] Done [ 10002 ]
          nlust Nate Lust made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          swinbank John Swinbank made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          swinbank John Swinbank made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          fred3m Fred Moolekamp made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          fred3m Fred Moolekamp made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          fred3m Fred Moolekamp made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          fred3m Fred Moolekamp made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          fred3m Fred Moolekamp made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          Parejkoj John Parejko made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          Parejkoj John Parejko made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          spietrowicz Steve Pietrowicz made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          rowen Russell Owen made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          rowen Russell Owen made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]
          tjenness Tim Jenness made changes -
          Remote Link This issue links to "Page (Confluence)" [ 14218 ] This issue links to "Page (Confluence)" [ 14218 ]

            People

            Assignee:
            rowen Russell Owen
            Reporter:
            rowen Russell Owen
            Reviewers:
            Steve Pietrowicz
            Watchers:
            Andy Salnikov, Nate Pease [X] (Inactive), Russell Owen, Steve Pietrowicz, Tim Jenness
            Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.