Skip to content

Add pysam and continue if test fetching errors#128

Merged
rhpvorderman merged 1 commit intogalaxyproject:masterfrom
mvdbeek:add_pysam_continue_if_test_fetching_errors
Feb 15, 2021
Merged

Add pysam and continue if test fetching errors#128
rhpvorderman merged 1 commit intogalaxyproject:masterfrom
mvdbeek:add_pysam_continue_if_test_fetching_errors

Conversation

@mvdbeek
Copy link
Copy Markdown
Member

@mvdbeek mvdbeek commented Apr 25, 2019

No description provided.

@mvdbeek mvdbeek force-pushed the add_pysam_continue_if_test_fetching_errors branch from 74ae671 to 7738e94 Compare April 25, 2019 10:35
@rhpvorderman
Copy link
Copy Markdown
Contributor

Pysam? Do we want ephemeris to have anything to do with bioinformatics content? Isn't that out of scope for this project?

@mvdbeek
Copy link
Copy Markdown
Member Author

mvdbeek commented Apr 26, 2019

Ephemeris does tool testing, and this is needed to test that outputs match.

@rhpvorderman
Copy link
Copy Markdown
Contributor

rhpvorderman commented Apr 26, 2019

But do we then eventually not also need Biopython and cyvcf2 etc?

@mvdbeek
Copy link
Copy Markdown
Member Author

mvdbeek commented Apr 26, 2019

No, this is only needed so that we can verify the bam and cram files match. Most other formats are text or compressed text based, no special tools needed there. I can move it to extras_requires though, then you'll not get it by default. If we don't have pysam and we run tool tests we can log a warning.

@rhpvorderman
Copy link
Copy Markdown
Contributor

No, this is only needed so that we can verify the bam and cram files match. Most other formats are text or compressed text based, no special tools needed there. I can move it to extras_requires though, then you'll not get it by default. If we don't have pysam and we run tool tests we can log a warning.

Thank you for explaining. I was worried that a lot of dependencies would be added to ephemeris, which could give problems down the road. But BAM files are indeed one of the few things that cannot be tackled by the standardlib. The extras_requires seems a good solution. On the other hand I think you are in a much better position to judge. If this is needed for a lot of tool tests then it is probably good to have this by default.

I just did a check on pysam, and it only depends on cython, which depends on nothing. So adding it as a default dependency does not create dependency hell for tools which depend on ephemeris. The only one that I know of is planemo, and that probably needs the tool testing functionality.

@martenson
Copy link
Copy Markdown
Member

@mvdbeek ready to take this out of draft?

@martenson
Copy link
Copy Markdown
Member

This is mostly superseded by #137

The pysam requirement can and probably should be managed outside of Ephemeris. Thanks for the insights @rhpvorderman

@mvdbeek
Copy link
Copy Markdown
Member Author

mvdbeek commented Feb 15, 2021

Not sure why this was closed, we should still do it.

@mvdbeek mvdbeek reopened this Feb 15, 2021
@mvdbeek mvdbeek force-pushed the add_pysam_continue_if_test_fetching_errors branch from 7738e94 to 15fe0bf Compare February 15, 2021 10:14
@mvdbeek mvdbeek marked this pull request as ready for review February 15, 2021 10:14
@rhpvorderman
Copy link
Copy Markdown
Contributor

I disabled the codacy check. It is slow and not really necessary since we can lint on the actions CI.

@rhpvorderman rhpvorderman merged commit 3be983e into galaxyproject:master Feb 15, 2021
@mvdbeek
Copy link
Copy Markdown
Member Author

mvdbeek commented Feb 15, 2021

Thank you @rhpvorderman!

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