Clinical-Genomics / scout

VCF visualization interface
https://clinical-genomics.github.io/scout
BSD 3-Clause "New" or "Revised" License
152 stars 46 forks source link

Panels do not handle sub-versions correctly #4986

Open Jakob37 opened 1 month ago

Jakob37 commented 1 month ago

Describe the bug

When sub-versioning a panel 1.10 or 1.20 the version is handled as a float, truncating the zero.

Furthermore, version 1.9 seems to be ranked "higher" than for instance 1.11, making me think it is doing a numerical comparison to check the latest panel.

Here, adding first 1.2 and then 1.20. You see that it checks for whether 1.2 exists when adding 1.20 and aborting as it already is there.

panel_versioning

After adding also version 1.11, 1.9 still shows up as the most recent version. (I verified that after adding 1.91, it is indeed updated to show 1.91).

scout_panel_view

To Reproduce

Create a panel with version 1.1, and then 1.10 to observe the first issue.

Then create a panel with 1.9 and subsequently 1.11 to observe the second issue.

Additional context

I am running Scout version 4.90.1 during testing.

dnil commented 1 month ago

That seems very plausible. Adding single panel version via CLI is very rare here nowadays, so can easily see this slipping past.

northwestwitch commented 1 month ago

Have you started with this one @dnil ? Otherwise I can!

northwestwitch commented 1 month ago

There is no way that panel 1.20 can be saved into the database as a float, because it will always become 1.2. I guess it needs to be cast as a decimal (pymongo has a specific type named Decimal128), but then we need to make sure that floats and decimals coexist and become comparable throughout the code, both python and jinja.

northwestwitch commented 1 month ago

Meh, I don't really know how to fix this without messing up the database. We either:

  1. Decide that from now on all panel versions become decimals instead of floats, and then we run a script to convert all panel versions to decimals in the database and adjust the scout code accordingly -- I don't like this option because it has the potential to introduce bugs, among other issues, like the fact that it is incorrect anyway to render a version number as a float, because what if you wanted to have version 1.20.1 ? --
  2. Force the user that is updating panels using the command line to provide a version which will work fine as a float
  3. Remove the --version entirely as an option from the command line, so that versions are handled by scout itself

Option 3 seems the easiest to me. Any suggestions?

Jakob37 commented 1 month ago

Meh, I don't really know how to fix this without messing up the database. We either:

1. Decide that from now on all panel versions become decimals instead of floats, and then we run a script to convert all panel versions to decimals in the database and adjust the scout code accordingly _-- I don't like this option because it has the potential to introduce bugs, among other issues, like the fact that it is incorrect anyway to render a version number as a float, because what if you wanted to have version 1.20.1 ? --_

2. Force the user that is updating panels using the command line to provide a version which will work fine as a float

3. Remove the --version entirely as an option from the command line, so that versions are handled by scout itself

Option 3 seems the easiest to me. Any suggestions?

Hmm. Assigning version directly have been useful to us, when restoring panels which accidentally was removed. Sounds a bit unfortunate to completely remove it.

For me, point 2 sounds better. And being very clear in the scout load panel interface on the expected format. I guess the issue I encountered here could be seen more like a misunderstanding of the expected format than a bug.

Might still lead to misunderstanding with 2.1 vs 2.01 though 🤔 Not sure how to get around that without more drastic changes.

northwestwitch commented 1 month ago

Hmm. Assigning version directly have been useful to us, when restoring panels which accidentally was removed. Sounds a bit unfortunate to completely remove it.

For me, point 2 sounds better. And being very clear in the scout load panel interface on the expected format. I guess the issue I encountered here could be seen more like a misunderstanding of the expected format than a bug.

Might still lead to misunderstanding with 2.1 vs 2.01 though 🤔 Not sure how to get around that without more drastic changes.

Mmm this is a clear example of poor judgment when the functionality has been created. The easiest would have been accepting only integers, which is what happens when you create/update panels using the web interface.

I've been testing a bit on my new PR but there is no really a way around the problem that 1.2 is the same numbers as 1.20 if you are either using floats or decimals. Things change if you use strings, but you don't want to go there because then you'll lose the possibility of sorting by numerical version.

I need to think a bit more about this