Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-42469: Update testers and fan-out for the nextVisit schema change #114

Merged
merged 4 commits into from
Jan 24, 2024

Conversation

hsinfang
Copy link
Collaborator

No description provided.

Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried about the schema ID, otherwise looks good!

detector: int
private_sndStamp: float # time of visit publication; TAI in unix seconds

def __str__(self):
"""Return a short string that disambiguates the visit but does not
include "metadata" fields.
"""
return f"(instrument={self.instrument}, groupId={self.groupId}, survey={self.survey} " \
return f"(groupId={self.groupId}, survey={self.survey} " \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's still useful to keep instrument as one of the "identifier" fields, even if the activator does almost immediately filter on it...

@@ -220,7 +220,7 @@ def send_next_visit(url, group, visit_infos):
f"filters: {info.filters} ra: {info.position[0]} dec: {info.position[1]} "
f"instrument: {info.instrument} survey: {info.survey}")
records_level = dict(value=asdict(info))
value_schema_level = dict(value_schema_id=1, records=[records_level])
value_schema_level = dict(value_schema_id=97, records=[records_level])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried about this arbitrary-looking number. Will we have to update this every time something changes in the dev Kafka?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems yes. I'm also worried about the not-very-well-documented update process related to Sasquatch redeployment and schema registry changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might alternatively send the messages with the full schema every time (a long string with all fields, their types. The value_schema on Sasquatch doc). I'm not sure that'd be better in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, if that's not what the summit is doing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't track down what summit does and will just continue to use schema id for now. I'll add some more docs in DM-42618

The field "instrument" was added to the schema of summit nextVist
messages. The fan-out service no longer needs to add the
"instrument" field to the fanned-out messages.
This commit adjusts the visit classes to reflect the schema change.
Testers use these classes to simulate the observations.
@hsinfang hsinfang force-pushed the tickets/DM-42469 branch 3 times, most recently from 8e46ac1 to c92a60d Compare January 18, 2024 23:35
Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed your re-review request somehow. startTime looks good, and I think it reflects typical AuxTel observations.

A new schema was added to the schema registry for test.next-visit,
which is used by dev testers. This new schema follows the recent
change of the summit nextVisit and contains the "instrument" field.
@hsinfang hsinfang force-pushed the tickets/DM-42469 branch 8 times, most recently from 0234a59 to 262375b Compare January 24, 2024 00:18
It was found that sometimes output products existed in the local
repo but were not transferred back to the central repo, because
the "safe type list" was outdated and had only types before any
processing (raw and calib). Refresh for the updated state.
@hsinfang hsinfang force-pushed the tickets/DM-42469 branch 2 times, most recently from 75e9e66 to d3249fb Compare January 24, 2024 21:57
If a dataset type isn't known to the local repo, the quantum
graph building now fails with MissingDatasetTypeError. It
was ignored in the past. This scenario is possible in cases
such as a fresh local repo is used and the incoming image
doesn't have matching templates. We want it to fall back to
try the next pipeline. If the repo knows the template's
dataset type but no matching templates exist, it still
generate an empty quantum graph.
@hsinfang hsinfang merged commit 217c987 into main Jan 24, 2024
5 checks passed
@hsinfang hsinfang deleted the tickets/DM-42469 branch January 24, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants