Skip to content

Enforce consistent preferred_event and t_0 update

Albert Zhang requested to merge albertcheng.zhang/gwcelery:enforce-t_0-update into main

Addresses #647 (closed) with future-proofing.

I kind of think it's best for GraceDB's to not store t_0 separately and instead pull it from the preferred event data. If we want to keep it consistent on Celery's end though, this is the best way I could come up with.

Previously, gracedb.update_superevent was able to be called with only one of preferred_event or t_0, and not the other. This leads to inconsistent t_0 and preferred_event gpstime on gracedb.

Going through the call sites,

  • in gwcelery/tasks/raven.py, preferred_event is never updated
  • in gwcelery/tasks/orchestrator.py and gwcelery/tasks/superevents.py preferred_event is only updated with the full event dictionary in scope.
  • in gwcelery/tasks/view.py preferred_event is updated without the event dictionary without an update to t_0, but it shouldn't be!

This patch

  • renames the old, unsafe gracedb.update_superevent to gracedb._update_superevent_raw
  • creates a new gracedb.update_superevent task that takes the event dictionary as its first argument, allowing chaining from tasks that return an event dictionary
  • adds a call to gracedb.get_event in the paths in gwcelery/tasks/view.py to get the correct event dictionary before chaining into gracedb.update_superevent
  • modifies the pre-existing callsites to use the new API
  • updates tests that mocked gracedb.update_superevent to gracedb._update_superevent_raw, making some of them stricter by testing for the correct preferred_event and t_0 in the call
  • adds a new test tests_tasks_superevents.py::test_parse_trigger_cbc_6 that tests the case where both the event dictionary, t_start and t_end are passed together (partially overlapping, more significant CBC event)
  • fixes the "both incomplete" branch in tests_tasks_superevents.py::test_update_preferred_event to correctly assume create_label is NOT called. this branch was previously unreachable due to the lack of a parametrize argument list for that case.

Merge request reports