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

Fix ivar trim's amplicon info on the fly generation #4431

Merged
merged 2 commits into from Mar 20, 2022

Conversation

wm75
Copy link
Contributor

@wm75 wm75 commented Feb 28, 2022

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)

Due to an option name mismatch auto-generation of the amplicon info file never happened in previous versions.

Due to an option name mismatch auto-generation of the amplicon info file
never happened in previous versions.
@bgruening
Copy link
Member

Mh, BAM files are compared vis sim_size, but we can compare them natively afaik and probably should do that.

@wm75
Copy link
Contributor Author

wm75 commented Feb 28, 2022

Yes, but for now I'm still totally puzzled why those unrelated tests are failing. There has been a new build of the bioconda package since the last update so maybe some unpinned dependency?

@bgruening
Copy link
Member

Yes, but for now I'm still totally puzzled why those unrelated tests are failing. There has been a new build of the bioconda package since the last update so maybe some unpinned dependency?

could be, we rolled out new htslib version

@wm75
Copy link
Contributor Author

wm75 commented Mar 2, 2022

Yeah, so it seems that either the htslib or the samtools update in the new build of the ivar conda package are causing the slight difference in the variant call stats produced by ivar variants.
Now what is the proper way to avoid such surprises in the future. Put a fixed version of htslib in the requirements, or of htslib and samtools?

@mvdbeek
Copy link
Member

mvdbeek commented Mar 2, 2022

Those tests should be running in a container, so differences there would be rather surprising (unless the container wasn't built on time)

@mvdbeek
Copy link
Member

mvdbeek commented Mar 2, 2022

Oh, I'm sure this is galaxyproject/galaxy#13460 as well.

@mvdbeek
Copy link
Member

mvdbeek commented Mar 3, 2022

Yep, that was it, green after merging galaxyproject/galaxy#13468

@wm75
Copy link
Contributor Author

wm75 commented Mar 3, 2022

Thanks @mvdbeek!
I guess ivar variants still deserves an explicit samtools requirement since it uses samtools mpileup on the command line?
Would do this in a separate PR though since requiring the latest version would also reintroduce the diffs causing the tests to fail, and that isn't really the scope here.

@mvdbeek
Copy link
Member

mvdbeek commented Mar 3, 2022

As long as samtools is installed I wouldn't add the requirement. If we want to totally pin down environments we should decide to do that across the board for everything. The idea was that we have containers for that purpose, but we could also decide to ship environment.yml files or generate them from the built containers.

@bgruening
Copy link
Member

So add samtools?

@bgruening bgruening merged commit 27f076c into galaxyproject:master Mar 20, 2022
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.

None yet

3 participants