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

reject new exposurelog messages with invalid obs_id

    XMLWordPrintable

    Details

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

      Description

      The original idea was to reject new messages with invalid obs_id (if is_new true; else reject when the butler finds no matching exposures). That code got reviewed and approved (thanks for a very helpful review Tim Jenness). Then we decided it would be much safer to require that any exposures exist in the registry; it is quite difficult to prevent invalid entries any other way.

      So for now I changed the code to deprecate and ignore the is_new flag to add_message, and always treat it as false. This simplifies the code (but required some work in test_add_messages.py – the other unit tests continue to add messages with invalid obs_id and instrument, since the messages are added directly rather than via the REST API and that is acceptable for unit tests).

      The long-term plan is to switch from using the butler to reading a new exposure database, once that is available (possibly later this spring), at which point it will once again be possible to annotate the current exposure(s) being taken.

        Attachments

          Issue Links

          relates to

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-37962 Deploy exposurelog 0.9.7

          • Done

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-37855 Sorting of dimension records no longer allows order by ID

          • Done

            Activity

            Ascending order - Click to sort in descending order
            Hide
            Permalink
            rowen Russell Owen added a comment - - edited

            Other fixes:

            • find_exposures was applying sorting and limit/offset in the wrong order; sort first.
            • fixed an outdated help string.
            • modernize dependencies, including sqlalchemy 2.0 and a fairly recent daf_butler (the newest has a sorting issue DM-37855 so we'll hold off on that for now).

            Pull requests:

            Show
            rowen Russell Owen added a comment - - edited Other fixes: find_exposures was applying sorting and limit/offset in the wrong order; sort first. fixed an outdated help string. modernize dependencies, including sqlalchemy 2.0 and a fairly recent daf_butler (the newest has a sorting issue DM-37855 so we'll hold off on that for now). Pull requests: https://github.com/lsst-sqre/exposurelog/pull/68 https://github.com/lsst-sqre/phalanx/pull/1937
            Hide
            Permalink
            tjenness Tim Jenness added a comment -

            Looks good. Minor comments on PR.

            Show
            tjenness Tim Jenness added a comment - Looks good. Minor comments on PR.
            Hide
            Permalink
            rowen Russell Owen added a comment - - edited

            Tim Jenness Thank you for a very helpful review. I applied all your suggested changes (except one to a regex, with an explanation).

            However, I then decided that it was not practical to really guard against invalid (instrument, obs_id) combinations (additional tests were needed and it was getting really complicated) so Merlin Fisher-Levine and I decided it would be better, for now, to disable the ability to enter messages for exposures that have not yet been ingested. The plan is to switch exposurelog to use the upcoming exposure database. If that database has information about exposures being taken (and here is one use case for that) then this restriction will vanish when that update is done.

            So I updated the code again (simplifying it) and put it back into review. Wouter van Reeven kindly agreed to review the next iteration (after offering it to Tim Jenness first). The new code ignores the is_new flag to add_message, and documents it as deprecated and ignored.

            Show
            rowen Russell Owen added a comment - - edited Tim Jenness Thank you for a very helpful review. I applied all your suggested changes (except one to a regex, with an explanation). However, I then decided that it was not practical to really guard against invalid (instrument, obs_id) combinations (additional tests were needed and it was getting really complicated) so Merlin Fisher-Levine and I decided it would be better, for now, to disable the ability to enter messages for exposures that have not yet been ingested. The plan is to switch exposurelog to use the upcoming exposure database. If that database has information about exposures being taken (and here is one use case for that) then this restriction will vanish when that update is done. So I updated the code again (simplifying it) and put it back into review. Wouter van Reeven kindly agreed to review the next iteration (after offering it to Tim Jenness first). The new code ignores the is_new flag to add_message, and documents it as deprecated and ignored.
            Hide
            Permalink
            wvreeven Wouter van Reeven added a comment -

            Reviewed on GitHub.

            Show
            wvreeven Wouter van Reeven added a comment - Reviewed on GitHub.
            Hide
            Permalink
            rowen Russell Owen added a comment - - edited

            The code is ready to merge, but I am waiting for a version of lsst-daf-butler that is compatible with sqlalchemy 2.0. Apparently that should land within a day or two (though I may not have time to update until late next week).

            Once that happens I have to update the input requirements, rebuild the built requirements and run tests again. If all that passes, then I'll merge and deploy.

            If we were in more of a hurry I could just pin an earlier version of sqlalchemy, but I would rather go to 2.0, since the code in exposurelog is already compatible with it.

            Show
            rowen Russell Owen added a comment - - edited The code is ready to merge, but I am waiting for a version of lsst-daf-butler that is compatible with sqlalchemy 2.0. Apparently that should land within a day or two (though I may not have time to update until late next week). Once that happens I have to update the input requirements, rebuild the built requirements and run tests again. If all that passes, then I'll merge and deploy. If we were in more of a hurry I could just pin an earlier version of sqlalchemy, but I would rather go to 2.0, since the code in exposurelog is already compatible with it.
            Hide
            Permalink
            rowen Russell Owen added a comment -

            I will deploy the new code next sprint using DM-37962.

            Show
            rowen Russell Owen added a comment - I will deploy the new code next sprint using DM-37962 .

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Wouter van Reeven
              Watchers:
              Merlin Fisher-Levine, Russell Owen, Tim Jenness, Wouter van Reeven
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.