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

Package a new version of CFITSIO ( 3.47 / 3470 )

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: DM
    • Labels:
      None
    • Sprint:
      Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-11, Arch 2019-11-18, Arch 2019-11-25, Arch 2019-12-02, Arch 2019-12-09, Arch 2019-12-16
    • Team:
      Architecture

      Description

      Package a new version of CFITSIO. This will definitely cause some tests to fail and FITS serialization code in afw (and other packages) may need to be updated, especially metadata packages.

        Attachments

          Issue Links

            Activity

            bvan Brian Van Klaveren created issue -
            bvan Brian Van Klaveren made changes -
            Field Original Value New Value
            Epic Link DM-20104 [ 319096 ]
            bvan Brian Van Klaveren made changes -
            Link This issue is triggered by RFC-640 [ RFC-640 ]
            bvan Brian Van Klaveren made changes -
            Link This issue is blocked by DM-21991 [ DM-21991 ]
            bvan Brian Van Klaveren made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            ktl Kian-Tat Lim made changes -
            Sprint Arch 2019-10-21 [ 968 ] Arch 2019-10-21, Arch 2019-10-22 [ 968, 971 ]
            ktl Kian-Tat Lim made changes -
            Sprint Arch 2019-10-21, Arch 2019-11-04 [ 968, 971 ] Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-5 [ 968, 971, 973 ]
            ktl Kian-Tat Lim made changes -
            Sprint Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-11 [ 968, 971, 973 ] Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-11, Arch 2019-11-12 [ 968, 971, 973, 976 ]
            ktl Kian-Tat Lim made changes -
            Sprint Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-11, Arch 2019-11-18 [ 968, 971, 973, 976 ] Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-11, Arch 2019-11-18, Arch 2019-11-19 [ 968, 971, 973, 976, 979 ]
            ktl Kian-Tat Lim made changes -
            Sprint Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-11, Arch 2019-11-18, Arch 2019-11-25 [ 968, 971, 973, 976, 979 ] Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-11, Arch 2019-11-18, Arch 2019-11-25, Arch 2019-11-26 [ 968, 971, 973, 976, 979, 982 ]
            ktl Kian-Tat Lim made changes -
            Sprint Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-11, Arch 2019-11-18, Arch 2019-11-25, Arch 2019-12-02 [ 968, 971, 973, 976, 979, 982 ] Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-11, Arch 2019-11-18, Arch 2019-11-25, Arch 2019-12-02, Arch 2019-12-3 [ 968, 971, 973, 976, 979, 982, 988 ]
            ktl Kian-Tat Lim made changes -
            Sprint Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-11, Arch 2019-11-18, Arch 2019-11-25, Arch 2019-12-02, Arch 2019-12-09 [ 968, 971, 973, 976, 979, 982, 988 ] Arch 2019-10-21, Arch 2019-11-04, Arch 2019-11-11, Arch 2019-11-18, Arch 2019-11-25, Arch 2019-12-02, Arch 2019-12-09, Arch 2019-12-10 [ 968, 971, 973, 976, 979, 982, 988, 991 ]
            Hide
            bvan Brian Van Klaveren added a comment -

            This is packaged with a few mitigations in the unit tests of afw and pipe_tasks (both one-line fixes). This is enough to build lsst_distrib.

            Show
            bvan Brian Van Klaveren added a comment - This is packaged with a few mitigations in the unit tests of afw and pipe_tasks (both one-line fixes). This is enough to build lsst_distrib.
            bvan Brian Van Klaveren made changes -
            Reviewers John Swinbank [ swinbank ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            bvan Brian Van Klaveren added a comment -

            Jenkins build is here:
            https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/31486/pipeline/48/

            Just to reiterate - it's not clear what effects this may have on user code, but we plan to switch to the conda-forge cfitsio imminently, which will be the same version, so this is a brief stepping stone before that.

            Show
            bvan Brian Van Klaveren added a comment - Jenkins build is here: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/31486/pipeline/48/ Just to reiterate - it's not clear what effects this may have on user code, but we plan to switch to the conda-forge cfitsio imminently, which will be the same version, so this is a brief stepping stone before that.
            Hide
            swinbank John Swinbank added a comment -

            Thanks; I'll take a look.

            I see this is blocked by DM-21991, which is marked as “to do”. Have you actually resolved the DM-21991 issues on this ticket?

            Show
            swinbank John Swinbank added a comment - Thanks; I'll take a look. I see this is blocked by DM-21991 , which is marked as “to do”. Have you actually resolved the DM-21991 issues on this ticket?
            Hide
            bvan Brian Van Klaveren added a comment -

            Yes I think I we can remove that blocking link and just close that ticket with this ticket - I thought the work would be much more involved.

            The two one-line uppercasing in the test cases (in afw and pipe_tasks) fixes the issues from DM-21991, although a more comprehensive fix (which I think involves changes to PropertySet) is expected in the future.

            Show
            bvan Brian Van Klaveren added a comment - Yes I think I we can remove that blocking link and just close that ticket with this ticket - I thought the work would be much more involved. The two one-line uppercasing in the test cases (in afw and pipe_tasks) fixes the issues from DM-21991 , although a more comprehensive fix (which I think involves changes to PropertySet) is expected in the future.
            bvan Brian Van Klaveren made changes -
            Link This issue relates to DM-24376 [ DM-24376 ]
            bvan Brian Van Klaveren made changes -
            Link This issue is blocked by DM-21991 [ DM-21991 ]
            bvan Brian Van Klaveren made changes -
            Link This issue relates to DM-21991 [ DM-21991 ]
            Hide
            bvan Brian Van Klaveren added a comment -

            DM-21991 has been closed in light that mitigations are on this ticket, and a new ticket for work which would warn when issues like this might pop up has been created - DM-24376.

            Show
            bvan Brian Van Klaveren added a comment - DM-21991 has been closed in light that mitigations are on this ticket, and a new ticket for work which would warn when issues like this might pop up has been created - DM-24376 .
            Hide
            swinbank John Swinbank added a comment -

            These changes are making me a little nervous. Can I check that I've properly grasped what's happening?

            As I understand it, when data is being written to FITS, HIERARCH keywords are being magically upper-cased. We then get the uppercase version of the keyword when we read it back. Thus, base_CircularApertureFlux_radii becomes BASE_CIRCULARAPERTUREFLUX_RADII on read. Right?

            That seems problematic — it means that the data no longer cleanly round trips, and the consumer has to know whether their data has previously been written to FITS or not (or, alternatively I guess, just assume it might have been and sprinkle calls to upper across their code). That seems unfortunate.

            Off the top of my head, preferable might be to:

            • Modify PropertySet (or List, or whatever the relevant container is) to uppercase everything which might be serialized to FITS as soon as it's added to the PropertySet, so at least the representation doesn't vary with round-tripping;
            • Ensure that we automatically upper-case everything we read, so that, on read, the results look the same regardless of whether the data was persisted with an old or a new version of CFITSIO.

            (And even that seems pretty ugly, to be honest.)

            Am I worried about nothing? Can you explain to my why this isn't a big deal?

            Show
            swinbank John Swinbank added a comment - These changes are making me a little nervous. Can I check that I've properly grasped what's happening? As I understand it, when data is being written to FITS, HIERARCH keywords are being magically upper-cased. We then get the uppercase version of the keyword when we read it back. Thus, base_CircularApertureFlux_radii becomes BASE_CIRCULARAPERTUREFLUX_RADII on read. Right? That seems problematic — it means that the data no longer cleanly round trips, and the consumer has to know whether their data has previously been written to FITS or not (or, alternatively I guess, just assume it might have been and sprinkle calls to upper across their code). That seems unfortunate. Off the top of my head, preferable might be to: Modify PropertySet (or List , or whatever the relevant container is) to uppercase everything which might be serialized to FITS as soon as it's added to the PropertySet , so at least the representation doesn't vary with round-tripping; Ensure that we automatically upper-case everything we read, so that, on read, the results look the same regardless of whether the data was persisted with an old or a new version of CFITSIO. (And even that seems pretty ugly, to be honest.) Am I worried about nothing? Can you explain to my why this isn't a big deal?
            tjenness Tim Jenness made changes -
            Watchers Brian Van Klaveren, John Swinbank [ Brian Van Klaveren, John Swinbank ] Brian Van Klaveren, John Swinbank, Kian-Tat Lim, Tim Jenness [ Brian Van Klaveren, John Swinbank, Kian-Tat Lim, Tim Jenness ]
            Hide
            tjenness Tim Jenness added a comment -

            There is a philosophical debate over whether PropertyList is representing FITS headers or it is sometimes serializing through FITS headers. You could change PropertyList to force upper casing on set. We thought it was a better short term fix to fix the tests since they are explicitly testing round tripping to FITS.

            There is a ticket open for warning if an attempt is made to serialize a lower case key to FITS.

            Show
            tjenness Tim Jenness added a comment - There is a philosophical debate over whether PropertyList is representing FITS headers or it is sometimes serializing through FITS headers. You could change PropertyList to force upper casing on set. We thought it was a better short term fix to fix the tests since they are explicitly testing round tripping to FITS. There is a ticket open for warning if an attempt is made to serialize a lower case key to FITS.
            Hide
            swinbank John Swinbank added a comment - - edited

            I don't want to get into a philosophical debate... but it seems like a substantial change to go from “PropertyList transparently round-trips through FITS” to “You can save a PropertyList to FITS, but when you load it again you won't get back exactly what you stored”. Maybe that's fine, but it feels like it's RFC worthy...?

            Show
            swinbank John Swinbank added a comment - - edited I don't want to get into a philosophical debate... but it seems like a substantial change to go from “ PropertyList transparently round-trips through FITS” to “You can save a PropertyList to FITS, but when you load it again you won't get back exactly what you stored”. Maybe that's fine, but it feels like it's RFC worthy...?
            Hide
            tjenness Tim Jenness added a comment -

            We can't keep using the old cfitsio for ever so we options are only allow upper case in PropertyList, refuse to write PropertyList entries that have lower cased keys, or somehow try to encode the real key in the FITS file. The current motivation for this ticket is switching to conda-forge since our eups cfitsio is really old.

            Show
            tjenness Tim Jenness added a comment - We can't keep using the old cfitsio for ever so we options are only allow upper case in PropertyList, refuse to write PropertyList entries that have lower cased keys, or somehow try to encode the real key in the FITS file. The current motivation for this ticket is switching to conda-forge since our eups cfitsio is really old.
            Hide
            swinbank John Swinbank added a comment -

            I see all that, but at the same time this seems like a breaking and confusing, albeit minor, interface change. It may be that it's sufficiently minor that nobody will care, but I'd be more comfortable if we could reach consensus on that — at least with a discussion at the CCB, and possibly though an RFC — before merging.

            Show
            swinbank John Swinbank added a comment - I see all that, but at the same time this seems like a breaking and confusing, albeit minor, interface change. It may be that it's sufficiently minor that nobody will care, but I'd be more comfortable if we could reach consensus on that — at least with a discussion at the CCB, and possibly though an RFC — before merging.
            tjenness Tim Jenness made changes -
            Link This issue is triggered by RFC-686 [ RFC-686 ]
            Hide
            bvan Brian Van Klaveren added a comment -

            DM-24376 was merged, and I think RFC-686 will be adopted. Soon after RFC-686 is adopted, I think we can merge this.

            A build with DM-24376 and this ticket was performed this morning:
            https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/31615/pipeline

            Show
            bvan Brian Van Klaveren added a comment - DM-24376 was merged, and I think RFC-686 will be adopted. Soon after RFC-686 is adopted, I think we can merge this. A build with DM-24376 and this ticket was performed this morning: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/31615/pipeline
            bvan Brian Van Klaveren made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            bvan Brian Van Klaveren made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            Hide
            bvan Brian Van Klaveren added a comment -

            This was solved in conda packaging and various other tickets.

            Show
            bvan Brian Van Klaveren added a comment - This was solved in conda packaging and various other tickets.

              People

              • Assignee:
                bvan Brian Van Klaveren
                Reporter:
                bvan Brian Van Klaveren
                Reviewers:
                John Swinbank
                Watchers:
                Brian Van Klaveren, John Swinbank, Kian-Tat Lim, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel