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

          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
          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).
          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?
          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.
          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?
          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 .
          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.
          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
          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.

            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.