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

AssembleCcdTask failure with Python 2

    XMLWordPrintable

    Details

    • Story Points:
      0.5
    • Epic Link:
    • Sprint:
      Alert Production S17 - 2
    • Team:
      Alert Production

      Description

      E.g. when running ip_isr's examples/runAssembleTask.py:

      Traceback (most recent call last):
        File "runAssembleTask.py", line 36, in <module>
          runAssembler()
        File "runAssembleTask.py", line 21, in runAssembler
          assembledExposure = assembleTask.assembleCcd(assemblyInput)
        File "/scratch/swinbank/ip_isr/python/lsst/ip/isr/assembleCcdTask.py", line 200, in assembleCcd
          ccd = next(assembleInput.values()).getDetector()
      TypeError: list object is not an iterator
      

      Looks like a bad conversion to Python 3: it assumes that dict.values() returns an iterator, but in Python 2 it returns a list. Simple (but ugly) fix:

      --- a/python/lsst/ip/isr/assembleCcdTask.py
      +++ b/python/lsst/ip/isr/assembleCcdTask.py
      @@ -197,7 +197,7 @@ class AssembleCcdTask(pipeBase.Task):
               ccd = None
               if hasattr(assembleInput, "has_key"):
                   # Get a detector object for this set of amps
      -            ccd = next(assembleInput.values()).getDetector()
      +            ccd = next(iter(assembleInput.values())).getDetector()
                   # Sent a dictionary of input exposures, assume one amp per key keyed on amp name
       
                   def getNextExposure(amp):
      

      Better suggestions welcome.

      Rather shocking that this wasn't caught by tests. Sufficiently shocking that I'm wondering if I missed something. I suggest that rather than just committing the above tweak, this ticket should include a test which demonstrates that the task actually runs. An easy way to do that might be to simply migrate a slightly modified version of the code from examples/ to tests/.

        Attachments

          Issue Links

            Activity

            No builds found.
            swinbank John Swinbank created issue -
            swinbank John Swinbank made changes -
            Field Original Value New Value
            Component/s ip_isr [ 10730 ]
            tjenness Tim Jenness made changes -
            Watchers John Swinbank, Mandeep Gill, Simon Krughoff [ John Swinbank, Mandeep Gill, Simon Krughoff ] John Parejko, John Swinbank, Mandeep Gill, Simon Krughoff [ John Parejko, John Swinbank, Mandeep Gill, Simon Krughoff ]
            Hide
            tjenness Tim Jenness added a comment -

            I'm pretty sure the code is broken on Python 3 as well. Note that the if-test in that block is broken and needs to be updated for python3 to use a mapping collection rather than checking for "has_key" (which does not exist for dicts on python3).

            I'm not completely convinced this worked on Python 2 with the old version of the code either as I can't see how python2 lists have a next() method:

            >>> a
            {1: 2, 3: 4}
            >>> hasattr(a, "has_key")
            True
            >>> a.values().next()
            Traceback (most recent call last):
              File "<stdin>", line 1, in <module>
            AttributeError: 'list' object has no attribute 'next'
            

            Is this a blocker for 13.0 release?

            Show
            tjenness Tim Jenness added a comment - I'm pretty sure the code is broken on Python 3 as well. Note that the if-test in that block is broken and needs to be updated for python3 to use a mapping collection rather than checking for "has_key" (which does not exist for dicts on python3). I'm not completely convinced this worked on Python 2 with the old version of the code either as I can't see how python2 lists have a next() method: >>> a {1: 2, 3: 4} >>> hasattr(a, "has_key") True >>> a.values().next() Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'list' object has no attribute 'next' Is this a blocker for 13.0 release?
            Hide
            swinbank John Swinbank added a comment - - edited

            The Python 2 version called itervalues(), not values(). The former returns an iterator rather than a list:

            >>> a = {1:2, 3:4}
            >>> a.values().next()
            Traceback (most recent call last):
              File "<stdin>", line 1, in <module>
            AttributeError: 'list' object has no attribute 'next'
            >>> a.itervalues().next()
            2
            

            Show
            swinbank John Swinbank added a comment - - edited The Python 2 version called itervalues() , not values() . The former returns an iterator rather than a list: >>> a = {1:2, 3:4} >>> a.values().next() Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: 'list' object has no attribute 'next' >>> a.itervalues().next() 2
            Hide
            tjenness Tim Jenness added a comment - - edited

            Sorry. Missed that bit when I was scanning the diffs.

            Show
            tjenness Tim Jenness added a comment - - edited Sorry. Missed that bit when I was scanning the diffs.
            mfisherlevine Merlin Fisher-Levine made changes -
            Link This issue blocks DM-8403 [ DM-8403 ]
            swinbank John Swinbank made changes -
            Link This issue relates to DM-8284 [ DM-8284 ]
            swinbank John Swinbank made changes -
            Link This issue blocks DM-8403 [ DM-8403 ]
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            Given that the fix for this is presumably trivial but is causing problems for people, could we decide on how to sort this and push the changes please?

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - Given that the fix for this is presumably trivial but is causing problems for people, could we decide on how to sort this and push the changes please?
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            PS This ticket is duplicated now by https://jira.lsstcorp.org/browse/DM-9370 which I just filed. Not sure whether to delete that or what, but presumable John Swinbank can help me clean up the mess :/

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - PS This ticket is duplicated now by https://jira.lsstcorp.org/browse/DM-9370 which I just filed. Not sure whether to delete that or what, but presumable John Swinbank can help me clean up the mess :/
            Hide
            krughoff Simon Krughoff added a comment - - edited

            Sorry that I missed this till now. I bet we can find someone to do it around here.

            Edit: Russell Owen said he'd take it on ASAP. We have a workshop tomorrow, but after that.

            Show
            krughoff Simon Krughoff added a comment - - edited Sorry that I missed this till now. I bet we can find someone to do it around here. Edit: Russell Owen said he'd take it on ASAP. We have a workshop tomorrow, but after that.
            rowen Russell Owen made changes -
            Assignee Russell Owen [ rowen ]
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            I have fixed on a ticket branch in DM-9370, but I don't know if this is the fix people want. It works, but it doesn't get around the fact that this should have been caught by tests. Not sure whether to submit for review or now...

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - I have fixed on a ticket branch in DM-9370 , but I don't know if this is the fix people want. It works, but it doesn't get around the fact that this should have been caught by tests. Not sure whether to submit for review or now...
            rowen Russell Owen made changes -
            Epic Link DM-8472 [ 28104 ]
            rowen Russell Owen made changes -
            Sprint Alert Production S17 - 2 [ 361 ]
            Labels SciencePipelines
            rowen Russell Owen made changes -
            Story Points 1 0.5
            Hide
            rowen Russell Owen added a comment -

            I fixed the code but posponed a unit test for DM-9374. The fix is straightforward. Please see if this works for you.

            Show
            rowen Russell Owen added a comment - I fixed the code but posponed a unit test for DM-9374 . The fix is straightforward. Please see if this works for you.
            rowen Russell Owen made changes -
            Reviewers Merlin Fisher-Levine [ mfisherlevine ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            swinbank John Swinbank made changes -
            Link This issue is duplicated by DM-9370 [ DM-9370 ]
            swinbank John Swinbank made changes -
            Link This issue relates to DM-9374 [ DM-9374 ]
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            Looks good to me!

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - Looks good to me!
            mfisherlevine Merlin Fisher-Levine made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              swinbank John Swinbank
              Reviewers:
              Merlin Fisher-Levine
              Watchers:
              John Parejko, John Swinbank, Mandeep Gill [X] (Inactive), Merlin Fisher-Levine, Russell Owen, Simon Krughoff, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.