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

chore(deps): Bump lapis-silo to 0.2.2 to allow empty input in silo-preprocessing #2073

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

corneliusroemer
Copy link
Contributor

@corneliusroemer corneliusroemer commented May 31, 2024

GenSpectrum/LAPIS-SILO#433

https://empty-input.loculus.org

Works!

image

Let's get it merged @Taepper soon, congrats!

@corneliusroemer corneliusroemer added the preview Triggers a deployment to argocd label May 31, 2024
@corneliusroemer corneliusroemer marked this pull request as draft May 31, 2024 10:00
@Taepper
Copy link
Contributor

Taepper commented May 31, 2024

You should retest now :)

@corneliusroemer
Copy link
Contributor Author

Retriggering

@theosanderson
Copy link
Member

You might want to delete the preview and recreate it

@corneliusroemer corneliusroemer force-pushed the empty-input branch 2 times, most recently from e7cc7ed to 96bf594 Compare June 2, 2024 18:14
@corneliusroemer corneliusroemer changed the title [don't merge] test lapis-silo PR empty input chore(deps): Bump lapis-silo to 0.2.2 to allow empty input in silo-preprocessing Jun 2, 2024
@corneliusroemer corneliusroemer marked this pull request as ready for review June 2, 2024 18:15
@corneliusroemer corneliusroemer added the review please PR waiting for final review label Jun 2, 2024
@corneliusroemer corneliusroemer enabled auto-merge (squash) June 2, 2024 18:19
@theosanderson
Copy link
Member

I still see 503 from https://lapis-empty-input.loculus.org/dummy-organism/sample/details is that what you expect because we haven't adjusted the thing that prepares the data to import?

@corneliusroemer
Copy link
Contributor Author

My gut feeling was that this was expected? What works now is silo preprocessing, it no longer crashes. But you're right @theosanderson, this problem doesn't look entirely solved. SILO/LAPIS shouldn't 503 when there's no data available. It should simply do what one expects when there's no data. Return nothing.

The PR mentions:

In particular the ingest with an empty ndjson (which does not have keys anymore) should not fail. This was challenging, because we never want to read the ndjson files into a table as a whole, but only the non-sequence columns of it, and the sequence columns should be compressed while reading them

@Taepper can you enlighten us?

@theosanderson
Copy link
Member

Are we actually preparing an empty dataset for SILO to import? My idea was that maybe we weren't, but if you say it was crashing before then I guess we were

@corneliusroemer
Copy link
Contributor Author

Yeah it crashed and as a result we didn't update the output but discarded. Now it doesn't crash, so we use the output. But now SILO-proper crashes. I guess we've pushed the error down one step, but still way to go to LAPIS 🙃 First SILO, then LAPIS. It's possible that SILO can actually deal with the empty input and it's now a LAPIS bug. I don't know. @chaoran-chen / @Taepper might be able to tell :)

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.

Approving as it seems ok to bump, regardless of this Q

@corneliusroemer corneliusroemer merged commit b380a36 into main Jun 3, 2024
8 checks passed
@corneliusroemer corneliusroemer deleted the empty-input branch June 3, 2024 00:10
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 review please PR waiting for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants