Fix: Prevent duplicate AMRFinderPlus database paths#7949
Conversation
Include db_version in data-manager output rows and make reruns skip databases whose stable data-table value is already registered. Bump the data-manager Galaxy version suffix once for the combined data-manager changes.
Filter duplicate database names in the wrapper data-table options and add a regression fixture/assertion for comma-joined database paths. Bump only the Galaxy build suffix for the wrapper.
The data manager exposes AMRFinderPlus database 4.0, but amrfindeplus itself is still tied to the v3. Because 4.0 was the first select option, tests that did not explicitly set a database version started exercising the unsupported v4 path and the duplicate-database test compared a known 3.12 value against a selected 4.0 database. Explicitly select the 3.12 test databases in the existing tests and remove the premature v4 data-manager test case. This keeps v4 available from the data manager, while avoiding attempting to test an unready end-to-end path.
|
Bump |
|
@paulzierep can you please have a look? |
|
|
|
Yes, only invocations. But if I install a new version of the DB manager and then run the new version, it will add a new row, even if one already exists. |
paulzierep
left a comment
paulzierep
left a comment
There was a problem hiding this comment.
mainly questions from my side
| # | ||
| # for example | ||
| amrfinderplus_V3.12_2024-05-02.2 V3.12-2024-05-02.2 3.12 ${__HERE__}/test-db | ||
| amrfinderplus_V3.12_2024-05-02.2 V3.12-2024-05-02.2 3.12 ${__HERE__}/test-db |
There was a problem hiding this comment.
this should not happen with the DM update, right ?
There was a problem hiding this comment.
if you need doublicated row for the tool test, I think you only need the example in the loc.test file.
There was a problem hiding this comment.
That's right, but I want to keep the fixes on both the tool wrapper and the DB manager side. Therefore, the ability to ignore doublets should also be tested for the AMRFinderPlus tool itself. I'll add an explicit test for this.
| </assert_contents> | ||
| </output> | ||
| </test> | ||
| <!-- Test_3 DB 4.0 2025-07-16.1 --> |
There was a problem hiding this comment.
why can the DM not download the latest DB, even if the tool is not wrapped yet, the download should work
There was a problem hiding this comment.
You are right, that should still be tested. I'll add one.
Make data manager tests download and check v4 databases, even as tool wrapper only accepts v3 databases. Add a dedicated AMRFinderPlus wrapper regression test for duplicate data-table rows before command rendering.
|
Thank you for the review - I have addressed the two unresolved review comments in the latest commit. |
3 participants
This PR contains four fixes to AMRFinderPlus and its database manager, fixing three separate bugs. They are made as three separate commits which can be reviewed independently.
The background is that we experienced AMRFinderPlus job failures. The executed command-line contained the option
--database '/users/data/Services/galaxy/server/tool-data/amrfinderplus-db/amrfinderplus_V3.12_2024-05-02.2,/users/data/Services/galaxy/server/tool-data/amrfinderplus-db/amrfinderplus_V3.12_2024-05-02.2', which is a bug (note it's the same path, joined with,)The underlying issue is that the data manager (DM) would add new rows to the output
.locfile every time it was invoked - including if new versions of the data manager was installed. This would result in identical rows in the.locfile.Then, AMRFinderPlus itself would query that database with
<filter type="static_value" value="3.12" column="db_version"/>, which could erroneously result in multiple values being selected.We fix this here in four steps:
db_versioncolumn to the DM output, which did not conform to the schema. This is a separate bug.FOR CONTRIBUTOR: