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(chore): Update to update to silo 0.3.0, lapis to 0.3.7, fix e2e tests #3077

Merged
merged 23 commits into from
Oct 26, 2024

Conversation

anna-parker
Copy link
Contributor

@anna-parker anna-parker commented Oct 25, 2024

resolves #

preview URL: https://fix-e2e.loculus.org/

Summary

Removee type: pango_lineage, also remove partitionBy option as this is only a lineage-feature (why?)

Screenshot

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by an appropriate test.

@anna-parker anna-parker added the preview Triggers a deployment to argocd label Oct 25, 2024
@corneliusroemer
Copy link
Contributor

Do we use that field anywhere? It's only in dummy I think? We could also just drop it there, it doesn't serve much purpose.

@fengelniederhammer
Copy link
Contributor

fengelniederhammer commented Oct 25, 2024

I'm quite sure you can just drop the pango lineage field (or make it a regular field - that's maybe even the better option, because there will be fewer changes).

@anna-parker
Copy link
Contributor Author

anna-parker commented Oct 25, 2024

@fengelniederhammer I tried dropping the type first so making it a regular field - but it seems that SILO sees the name as meaning a pango_lineage - so I got the same error. I didn't want to remove the entire field as I think this will mean having to change all e2e tests.

@fengelniederhammer
Copy link
Contributor

No, there is no meaning attached to field names in SILO. The SILO database config should look like:

    - name: pango_lineage
      type: string

(generateIndex: true is optional).
I'm very sure that this works (from a technical perspective).

@fengelniederhammer
Copy link
Contributor

I just read the error message careful enough:
partitionBy 'pangoLineage' must be of type STRING and needs 'generateLineageIndex' set

Simply remove partitionBy in that config. That fixes it.

@anna-parker
Copy link
Contributor Author

anna-parker commented Oct 25, 2024

@fengelniederhammer what does partitionBy do actually? - it looks like it just splits up the table - so why does it have to be a lineage now?

@anna-parker anna-parker marked this pull request as ready for review October 25, 2024 15:01
@anna-parker anna-parker changed the title Remove pangoLineage type, update to silo 0.3.0 to test compatibility feat(chore): Update to update to silo 0.3.0, fix e2e tests Oct 25, 2024
@fengelniederhammer
Copy link
Contributor

@fengelniederhammer what does partitionBy do actually? - it looks like it just splits up the table - so why does it have to be a lineage now?

https://lapis.cov-spectrum.org/open/v2/docs/maintainer-docs/references/database-configuration

It also had to be a lineage before. (Before: type: pango_lineage, now: generateLineageIndex: true).
It divides the data into partitions.

This provides optimizations:

  • It speeds up the queries, because we can process partitions in parallel.
  • We basically sort the data by lineage. Thus similar data is closer together. Thus similar mutations are closer together. That improves compression, because Roaring bitmaps compress better if the they are almost full or almost empty.

@anna-parker anna-parker changed the title feat(chore): Update to update to silo 0.3.0, fix e2e tests feat(chore): Update to update to silo 0.3.0, lapis to 0.3.7, fix e2e tests Oct 25, 2024
@corneliusroemer
Copy link
Contributor

Progress!

image

@corneliusroemer
Copy link
Contributor

corneliusroemer commented Oct 25, 2024

image

Breaking change in LAPIS?

image

On main we get this instead, FASTA not JSON

image

@corneliusroemer
Copy link
Contributor

corneliusroemer commented Oct 25, 2024

Ah the problem is that we pass some accept headers and they are used to change the default:

image

The default (if nothing is passed in accept header) is actually still FASTA

image

So it's only breaking in the sense that now you return what people ask for

@corneliusroemer corneliusroemer added the format_me Triggers github_actions to format website code on PR label Oct 25, 2024
@corneliusroemer
Copy link
Contributor

Nice, now it works!

image

@anna-parker anna-parker merged commit 335f12f into main Oct 26, 2024
19 checks passed
@anna-parker anna-parker deleted the fix_e2e branch October 26, 2024 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_me Triggers github_actions to format website code on PR preview Triggers a deployment to argocd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants