fix: Update to the latest version of nextclade. #1701
Conversation
| @@ -127,6 +128,13 @@ def cli( | |||
| # generalized case and we'll need to figure out how to handle that, | |||
| # but right now the workflow is hardcoded to always expecting dataset. | |||
| nextclade_dataset_name = target_pathogen.nextclade_dataset_name | |||
| # Nextclade 3.2.8 has new names for datasets vs the 2.1 names in the db. | |||
| new_nextclade_dataset_names = { | |||
There was a problem hiding this comment.
I'm not sure if normalizing the new names back to whatever the old one was is the right choice. I don't remember how the dataset name gets used, but if there's no logic based on the value elsewhere and its just being held so we know what reference was used, I think we shouldn't standardize to the old name.
There was a problem hiding this comment.
Oh, whoops, I see I misread the direction of the lookup var.
There was a problem hiding this comment.
yeah, I think this is just the minimal change possible -- we only use this value to dowload the right dataset via the nextclade cli, and nothing else changes anywhere in our system
There was a problem hiding this comment.
I honestly don't know: is there a reason we can't modify the row for the pathogens table so the nextclade_dataset_name for MPX is the new value instead? If that's doable, it seems preferable to go that way, but I also don't know if the refresh logic would freak out if old MPX and new MPX referenced different things.
There was a problem hiding this comment.
I did a skim through the code and it looks like this is the one-and-only place where the nextclade dataset name value is used, so it should be safe to update the db values instead - I'll update the PR
|
The update is in: Also tested SC2 samples and nothing is broken. Thank you both! |
Summary:
Demos:
Notes:
Checklist:
<base branch>