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

feat!: Pipeline version per organism (fixed) #3703

Merged
merged 4 commits into from
Feb 20, 2025
Merged

Conversation

chaoran-chen
Copy link
Member

@chaoran-chen chaoran-chen commented Feb 17, 2025

Original PR: #3534

fixes #3701

preview: https://debug-pipeline.loculus.org

There was an issue in the SQL definition of the view, causing the filter for only the current pipeline version to not work. This has been fixed. This PR also adds a test.

@chaoran-chen chaoran-chen added the preview Triggers a deployment to argocd label Feb 17, 2025
@chaoran-chen chaoran-chen added preview Triggers a deployment to argocd and removed preview Triggers a deployment to argocd labels Feb 17, 2025
@fhennig fhennig changed the title Debug pipeline PR feat!: Pipeline version per organism (fixed) Feb 17, 2025
…/CurrentProcessingPipelineTable.kt

Co-authored-by: Theo Sanderson <[email protected]>
@theosanderson
Copy link
Member

(I trust that Chaoran has looked at this in some detail - I don't feel able to add much with a review!)

@chaoran-chen chaoran-chen merged commit 67229b4 into main Feb 20, 2025
22 checks passed
@chaoran-chen chaoran-chen deleted the debug-pipeline branch February 20, 2025 12:11
@corneliusroemer
Copy link
Contributor

corneliusroemer commented Feb 20, 2025

@fhennig and @chaoran-chen Recalling our retro, I don't really feel approving this as is. How did you test the PR manually and did you test it with the latest commit?

I'm sure it fixes the duplication bug, but there might be other bugs hiding, especially given we know the initial PR was undertested.

If you haven't tested this, it might be good to do the following manually:

  1. Switch on persistence
  2. Let everything ingest and process
  3. Check processing version in get released data is how it's expected (i.e. highest of version in values.yaml)
  4. Add new processing versions in values.yaml
  5. Let everything process
  6. Check processing version is as expected in get released data
  7. Check silo preprocessing etc is happy

Ideally, the same thing would also be done on staging, where one would do similar to above but:

  1. Clone prod -> staging
  2. Switch to use this Loculus commit here
  3. Observe results are as expected
  4. Change values yaml, bumping a preprocessing version
  5. Observe results

@theosanderson
Copy link
Member

theosanderson commented Feb 20, 2025

@corneliusroemer I don't think these requests are unreasonable, but it would be really helpful to have raised them before merging. The authors asked for any input both here and on Slack 2 days ago

@theosanderson
Copy link
Member

(of course we can still check on staging)

@corneliusroemer
Copy link
Contributor

Thanks @theosanderson yes of course I could have raised this faster. For context, we've now discussed this on Slack: https://loculus.slack.com/archives/C05G172HL6L/p1740055353208289

@fhennig
Copy link
Contributor

fhennig commented Feb 21, 2025

About testing; like we said in yesterdays Retro, I'll make an effort to document much better what I have tested manually!

About the issue at hand: Your test idea:

Clone prod -> staging
Switch to use this Loculus commit here
Observe results are as expected
Change values yaml, bumping a preprocessing version
Observe results

Is good, and I did almost that (before this PR - when the other one was merged). I checked that the version is correctly upgraded in the table where the current version is kept, but I didn't check in the get-released data - I didn't consider that also the view was changed and that's where I missed the issue with the changed view. I guess 'Observe results' can mean many things, this time I would also specifically check get-released-data again.

My plan now is to test this again on staging like you suggested, and I will then document this here as well. I do not think it's necessary to revert this PR to do so.

If you object to doing things this way or have more things you would like to see tested, it would be great to add those rather sooner than later! Also happy to do this in a call with someone else together! More people -> More ideas for what to check.

I have moved this PR back into the 'In Progress' column on the board to signify that there is work to be done on it.

@fhennig
Copy link
Contributor

fhennig commented Feb 26, 2025

I've now done this test! PR is linked above, and it contains the description of what I did. I'll leave it open so the preview remains there for a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Triggers a deployment to argocd update_db_schema
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in silo-preprocessing on loculus main
5 participants