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

Ingest add revocation comment #2451

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

anna-parker
Copy link
Contributor

@anna-parker anna-parker commented Aug 19, 2024

resolves #

preview URL: https://ingest-add-revocation-com.loculus.org/

Summary

Add description of why sequence has been revoked if revoked due to grouping changes

Screenshot

PR Checklist

  • Use test data to confirm revocation commands work and add revocation details:
image

@anna-parker anna-parker added the preview Triggers a deployment to argocd label Aug 20, 2024
@anna-parker anna-parker marked this pull request as ready for review August 21, 2024 10:00
@anna-parker anna-parker removed the preview Triggers a deployment to argocd label Aug 21, 2024
ingest/README.md Outdated Show resolved Hide resolved
@corneliusroemer corneliusroemer added preview Triggers a deployment to argocd issue_needs_cleanup Need to remove before go-live and removed preview Triggers a deployment to argocd issue_needs_cleanup Need to remove before go-live labels Aug 29, 2024
@anna-parker
Copy link
Contributor Author

It would be nice to merge this before #2651

@anna-parker anna-parker added the review please PR waiting for final review label Aug 30, 2024
@anna-parker anna-parker added the preview Triggers a deployment to argocd label Aug 30, 2024
Copy link
Member

@theosanderson theosanderson left a comment

Choose a reason for hiding this comment

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

Looks good to me in terms of readability,etc.! Thanks! In terms of being confident it works:

(A) I presume you have tested it out in some way?
(B) At some point pretty soon we should add code tests for all of this - but it seems mean to do that in this PR specifically :)

Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

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

How is regroup defined? We could in principle not revoke If the change is simply addition of a new segment to an existing segment.

Have you tested revoke with something like: run ingest with sorted authors, then ingest with unsorted authors. That should show some revokes.

@anna-parker
Copy link
Contributor Author

anna-parker commented Sep 3, 2024

How is regroup defined?

The functions were added with ingest tests in #2372.

I presume you have tested it out in some way?

I have only tested with the test examples (and some modifications of the test examples) in the ingest tests/test_data_cchf folder. #2651 seems like a good test on actual data.

We could in principle not revoke If the change is simply addition of a new segment to an existing segment.

At the moment revoke_and_regroup will always revoke the old group and create a new group. I realized handling different cases gets very complicated quickly and as this is such an edge case anyways I didn't think it made sense to go into details yet. That is also why I made revoke_and_regroup only run with manual approval. We can look into the cases and handle them differently if we would like to.

@anna-parker anna-parker removed the preview Triggers a deployment to argocd label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review please PR waiting for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants