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

Minor fixes to tool_data_table_conf and related files for clair3 and artic #6790

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pvanheus
Copy link
Contributor

@pvanheus pvanheus commented Feb 22, 2025

FOR CONTRIBUTOR:

  • I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • License permits unrestricted use (educational + commercial)
  • This PR adds a new tool or tool collection
  • This PR updates an existing tool or tool collection
  • This PR does something else (explain below)

@@ -2,8 +2,8 @@
<tables>
<!-- Locations of clair3 model folders -->
<table name="model" comment_char="#">
<columns>value, name, platform, path, source</columns>
<file path="tool-data/model.loc" />
<columns>value, platform, sha256, path, source</columns>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very unfortunate, but you cannot change the columns of an existing data table.
You need also a new table name now.

Copy link
Contributor Author

@pvanheus pvanheus Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify - the table is not like the one described here - the canonical version of the table is the one described here: #6659 (there was something to create the table before this DM was created, even though it was referenced in the clair3 tool). So this brings the clair3 copy of the data table definition in line with the one made by the data manager.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I see (was wondering how I had missed this when reviewing the PR for the DM). Then this might work.
Not exactly sure how all the different versions of this file are treated by Galaxy though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly sure how all the different versions of this file are treated by Galaxy though.

This will create problems if someone already installed the version without the fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bernt-matthias will the problem be there if people installed the tool with the older version of the data table or only for those installing the DM (if, for example, a data table changed after the DM was installled)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but I guess/hope we can manage to clean things up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but @bernt-matthias you'd merge without a version bump then, correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Contributor

@wm75 wm75 Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, then let's do that. @pvanheus can you undo the two wrapper version bumps and I'll try to sort out the EU data tables?

Copy link
Contributor

@wm75 wm75 Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also seems that you haven't captured all the changes from #6796 (which unfortunately has been merged already) in this PR here (which means we cannot overwrite the +galaxy0 version of artic_minion here anymore). Can you please just rebase?

(No harm done with the artic_minion tool since that one always had the correct tool_data_table_conf data :-) but rebasing would still be good to make the situation less confusing. )

@@ -2,8 +2,8 @@
<tables>
<!-- Locations of clair3 model folders -->
<table name="model" comment_char="#">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name of the table is clair3_models. Can you update that please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for the artic_minion tool!

@wm75
Copy link
Contributor

wm75 commented Feb 26, 2025

@pvanheus another question while we're at it: have the model files ever changed their format?
Newly installed clair3_models are now also user-selectable for old versions of clair3 (like 0.1.11+galaxy0 installed on EU). Would they be compatible?

@pvanheus
Copy link
Contributor Author

@pvanheus another question while we're at it: have the model files ever changed their format? Newly installed clair3_models are now also user-selectable for old versions of clair3 (like 0.1.11+galaxy0 installed on EU). Would they be compatible?

The model files that are built by the clair3 group (available here) have not changed since October 2021, which is before the release date of any of the versions of clair3 used in Galaxy, so it seems that the model format is very stable and definitely stable across all Galaxy tool releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants