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

In ingest, populate the version comment field with a diff versus the previous version #2804

Open
corneliusroemer opened this issue Sep 16, 2024 · 5 comments
Labels
feature Feature proposal ingest Ingest pipeline

Comments

@corneliusroemer
Copy link
Contributor

corneliusroemer commented Sep 16, 2024

In ingest, we should add the diff between previous and new version to the version comment field. Ideally we'll do this before we merge:

@corneliusroemer corneliusroemer added feature Feature proposal ingest Ingest pipeline labels Sep 16, 2024
@theosanderson
Copy link
Member

theosanderson commented Sep 16, 2024

As Emma sort of mentioned in the chat on the call - I don't think an automated diff is desirable for version comments. I think these should be descriptive (like commit messages) and should describe intent. So a useful message here would be something like Author order used to be automatically resorted by Pathoplexus ingest pipeline, now instead it reflects INSDC order (see github.com/bla). I appreciate figuring out how to do that isn't necessarily easy, but if we can't do it I still wouldn't go for an automated diff as this message - I think if we want that we should do it with the comparison feature.

@corneliusroemer
Copy link
Contributor Author

I think ingest is special in this case, as it's automated. It's true that it'd be nicer to have a human description there when the change is due to ingest code changes. But a diff isn't a problem if the changes are due to upstream changes?

You're right that a diff would make the comment less useful.

But I don't know how to automatically add such a custom comment in ingest - we could do it manually in the db? Or we do it with configuration passed to ingest? We run it once with the version comment, then turn the comment off a few minutes later?

@theosanderson
Copy link
Member

theosanderson commented Sep 16, 2024

Yes, I was just thinking that a manual DB operation (pseudocode) UPDATE SET JSON.versionComment = 'bla' WHERE date_submitted IN LAST 3 HOURS would probably work pretty well.

I think in the case of upstream changes - which are not the case here! - then a useful automated comment might be Authors field updated on INSDC or something

@corneliusroemer
Copy link
Contributor Author

Right - so we are not opposed to version comment in principle, just in this case here it's best to add better context than a diff would give.

@chaoran-chen
Copy link
Member

As we plan to implement a diff viewer, I think that something that indicates that a change has been made automatically due to an update on INSDC makes sense. This would distinguish it from manual curation changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature proposal ingest Ingest pipeline
Projects
None yet
Development

No branches or pull requests

3 participants