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

PropertySet does not support values of None

    XMLWordPrintable

    Details

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

      Description

      In DM-5466, we needed to pass the results from ParseTask.getInfo() to the butler as a dataId. This is normally valid, since both are dictionaries, and even though getInfo() often contains extraneous entries that aren't relevant, the butler will usually ignore them. However, when parsing calibration files this dictionary contains some values that are set to None, since they will be filled in later. These extraneous keys are then placed in ButlerLocation.additionalData (butlerLocation.py:221), which throws an exception as it is a PropertySet and does not support python None as a value.

      DM-5466 has a work-around that strips these None values from the dictionary, but this is inelegant. The main driver for excluding None from PropertySet seems to compatibility with FITS headers. This seems like an unwarranted mixing of data model and persistence formats. Unless there is some advantage to not being able to store None in our dictionary-like objects, it seems preferable to shift the burden of accommodating FITS's peculiarities onto the persistence layer rather than PropertySet.

        Attachments

          Issue Links

            Activity

            No builds found.
            ctslater Colin Slater created issue -
            Hide
            tjenness Tim Jenness added a comment -

            I don't think FITS could be the justification here because FITS headers support undefined values.

            HEADER  =         / an undefined value
            

            Is the FITS standard scheme for storing an undefined value in a header (it has no derivable type). Historically, convincing header parses to handle this case has been tricky (the AST library does know how to do it though).

            Show
            tjenness Tim Jenness added a comment - I don't think FITS could be the justification here because FITS headers support undefined values. HEADER = / an undefined value Is the FITS standard scheme for storing an undefined value in a header (it has no derivable type). Historically, convincing header parses to handle this case has been tricky (the AST library does know how to do it though).
            Hide
            swinbank John Swinbank added a comment -

            Kian-Tat Lim — is there an Architecture concern here? I am happy to accept this work onto the backlog for the DRP team to address as long as you don't think it needs further discussion first.

            Show
            swinbank John Swinbank added a comment - Kian-Tat Lim — is there an Architecture concern here? I am happy to accept this work onto the backlog for the DRP team to address as long as you don't think it needs further discussion first.
            Hide
            ktl Kian-Tat Lim added a comment - - edited

            Adding None as a valid value for PropertySet entries will likely require adding an additional boolean to each entry as None has no type. Code will need to be added to handle the cases of creating a new entry with None value, setting a value of existing type to None, setting a value of None to a new value of some type, and likely preventing a value that has been set to one type from being set to a different type after having an intermediate None stored. Furthermore, handling of lists of values will have to be defined: Does None replace the entire list, or is it a list entry, further complicating type issues? I suggest the former matches the FITS expectation and is simpler.

            We also have to consider whether None-supporting PropertySet s and PropertyList s will be backward-compatible with previously-persisted objects. (If I or someone else could finish the YAML persistence for DM-4927, which is partly complete at daf_persistence branch u/ktlim/yamlStorage, that might help as well.)

            Show
            ktl Kian-Tat Lim added a comment - - edited Adding None as a valid value for PropertySet entries will likely require adding an additional boolean to each entry as None has no type. Code will need to be added to handle the cases of creating a new entry with None value, setting a value of existing type to None , setting a value of None to a new value of some type, and likely preventing a value that has been set to one type from being set to a different type after having an intermediate None stored. Furthermore, handling of lists of values will have to be defined: Does None replace the entire list, or is it a list entry, further complicating type issues? I suggest the former matches the FITS expectation and is simpler. We also have to consider whether None -supporting PropertySet s and PropertyList s will be backward-compatible with previously-persisted objects. (If I or someone else could finish the YAML persistence for DM-4927 , which is partly complete at daf_persistence branch u/ktlim/yamlStorage , that might help as well.)
            swinbank John Swinbank made changes -
            Field Original Value New Value
            Labels SciencePipelines
            Hide
            tjenness Tim Jenness added a comment -

            I think one of either this ticket or DM-8101 should be marked as a duplicate. They both want support for None as values in PropertyList/Set but they are coming at it from opposite ends (one wants it because of FITS and this one because of Python).

            Show
            tjenness Tim Jenness added a comment - I think one of either this ticket or DM-8101 should be marked as a duplicate. They both want support for None as values in PropertyList/Set but they are coming at it from opposite ends (one wants it because of FITS and this one because of Python).
            tjenness Tim Jenness made changes -
            Risk Score 0
            tjenness Tim Jenness made changes -
            Link This issue is duplicated by DM-8101 [ DM-8101 ]
            tjenness Tim Jenness made changes -
            Link This issue is triggered by RFC-239 [ RFC-239 ]
            tjenness Tim Jenness made changes -
            Assignee Tim Jenness [ tjenness ]
            tjenness Tim Jenness made changes -
            Labels SciencePipelines
            tjenness Tim Jenness made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            tjenness Tim Jenness made changes -
            Sprint Arch 2019-04-01 [ 899 ]
            Team Architecture [ 10304 ]
            tjenness Tim Jenness made changes -
            Story Points 3
            Hide
            tjenness Tim Jenness added a comment -

            Kian-Tat Lim I think this is ready for at least an initial review. It turned out to be really simple: register nullptr_t as a type and pybind11 automatically converts that to None.

            The handling of scalars works fine. Storing a None inside an array of ints (or hoping that getInt can return None) will I think require magic values inside the C++ layer that map to None in the python layer. I'm not sure it's worth it for now.

            Show
            tjenness Tim Jenness added a comment - Kian-Tat Lim I think this is ready for at least an initial review. It turned out to be really simple: register nullptr_t as a type and pybind11 automatically converts that to None. The handling of scalars works fine. Storing a None inside an array of ints (or hoping that getInt can return None) will I think require magic values inside the C++ layer that map to None in the python layer. I'm not sure it's worth it for now.
            tjenness Tim Jenness made changes -
            Reviewers Kian-Tat Lim [ ktl ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            tjenness Tim Jenness added a comment -

            Colin Slater do you know what I need to reverse from DM-5466 to test that None now works?

            Show
            tjenness Tim Jenness added a comment - Colin Slater do you know what I need to reverse from DM-5466 to test that None now works?
            Hide
            ctslater Colin Slater added a comment -

            I can't locate anything that looks like it would have been the culprit for filing this ticket; DM-5466 went through a few permutations so the final code that was merged might not have had the same problem we saw originally.

            Show
            ctslater Colin Slater added a comment - I can't locate anything that looks like it would have been the culprit for filing this ticket; DM-5466 went through a few permutations so the final code that was merged might not have had the same problem we saw originally.
            tjenness Tim Jenness made changes -
            Link This issue relates to DM-8101 [ DM-8101 ]
            tjenness Tim Jenness made changes -
            Link This issue relates to DM-8101 [ DM-8101 ]
            tjenness Tim Jenness made changes -
            Link This issue relates to DM-8100 [ DM-8100 ]
            tjenness Tim Jenness made changes -
            Link This issue is triggering DM-18864 [ DM-18864 ]
            tjenness Tim Jenness made changes -
            Reviewers Kian-Tat Lim [ ktl ] Jim Bosch, Kian-Tat Lim [ jbosch, ktl ]
            Hide
            tjenness Tim Jenness added a comment -

            Jim Bosch thanks for agreeing to look at this.

            Show
            tjenness Tim Jenness added a comment - Jim Bosch thanks for agreeing to look at this.
            Hide
            jbosch Jim Bosch added a comment -

            I think the changes for me to look at were on DM-18864, right?  Or should I look at the ones here, too?

            Show
            jbosch Jim Bosch added a comment - I think the changes for me to look at were on DM-18864 , right?  Or should I look at the ones here, too?
            Hide
            tjenness Tim Jenness added a comment -

            The changes were to look at this one. I referred you to DM-18864 because of my recent addition there to work around the problem here based on the Slack discussion this morning.

            Show
            tjenness Tim Jenness added a comment - The changes were to look at this one. I referred you to DM-18864 because of my recent addition there to work around the problem here based on the Slack discussion this morning.
            jbosch Jim Bosch made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            tjenness Tim Jenness made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            tjenness Tim Jenness made changes -
            Epic Link DM-16576 [ 234914 ]

              People

              Assignee:
              tjenness Tim Jenness
              Reporter:
              ctslater Colin Slater
              Reviewers:
              Jim Bosch, Kian-Tat Lim
              Watchers:
              Colin Slater, Jim Bosch, John Swinbank, Kian-Tat Lim, Meredith Rawls, Simon Krughoff, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.