loculus-project / loculus

An open-source software package to power microbial genomic databases
https://loculus.org
GNU Affero General Public License v3.0
37 stars 2 forks source link

feat(chore): Update to update to silo 0.3.0, lapis to 0.3.7, fix e2e tests #3077

Closed anna-parker closed 1 month ago

anna-parker commented 1 month ago

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

corneliusroemer commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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?

fengelniederhammer commented 1 month ago

@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:

corneliusroemer commented 1 month ago

Progress!

image
corneliusroemer commented 1 month ago
image

Breaking change in LAPIS?

image

On main we get this instead, FASTA not JSON

image
corneliusroemer commented 1 month ago

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 commented 1 month ago

Nice, now it works!

image