bgruening / galaxytools

:microscope::books: Galaxy Tool wrappers
MIT License
115 stars 220 forks source link

Update Nanopolish to v0.14 #1359

Closed tuncK closed 7 months ago

tuncK commented 7 months ago

I attempted to address the following:

a) Existing version provides incorrect result (0 usable event detected) on a recent in-house project. Allegedly an issue that is nanopolish version-dependent. b) Current wrapper does not explicitly handle input.fast5.tar.xz as input. c) Current wrapper fails linting.

bgruening commented 7 months ago

Otherwise this looks very cool!

tuncK commented 7 months ago

So what happens if I provide a x.tar.xz? I have such files in my history and no error was raised. image

bgruening commented 7 months ago

But as you see its not recognized as tar.xz datatype. I guess we would need to add this compression to Galaxy.

tuncK commented 7 months ago

OK, so I did a quick test by manually uploading files. gz is auto-decompressed whereas xz is not recognised at all. This is unlike the case I mentioned above, where the contents of the tarball were recognised properly. image

So how should I proceed in this case? a) I should Eliminate .xz from this tool input and you merge as is? b) We add .xz to allowed filetypes. How?

bgruening commented 7 months ago

Ideally, we add a new datatype similar to here: https://github.com/galaxyproject/galaxy/commit/f07c6a98431f1a4c2a2d81f99d3b54a23f0a901e a new class essentially.

Do you know if tools understand this tarball natively?

tuncK commented 7 months ago

Is this what you mean https://github.com/tuncK/galaxy/tree/tarxz? Where should I be sending the PR to: usegalaxy-eu/galaxy or galaxyproject/galaxy?

No, I dont think the tool itself supports any compression whatsoever, it only takes paths to fast5 files. That is why this wrapper needs to decompress them manually first.

bgruening commented 7 months ago

Yes! Pretty much!

Please sent this to upstream Galaxy and the release_23.1 branch

bgruening commented 7 months ago

:1st_place_medal:

tuncK commented 7 months ago

Any chance we can merge this without waiting for the .xz PR so that I can use this after Saturday's auto-update?

bgruening commented 7 months ago

ofc :)