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

Add fall back on SRA if ENA fails #5

Merged
merged 20 commits into from
Jun 1, 2022
Merged

Conversation

mbhall88
Copy link
Contributor

@mbhall88 mbhall88 commented May 28, 2022

As the title suggests, this PR adds functionality that will try from SRA if ENA fails.

I formatted the code with black sorry, so the PR looks more complicated than it is.

Feel free to not take any of this either. I won't be offended.

Added

  • -o shorthand option for --outdir
  • a flag -F/--only-provider, which supercedes --sra_only. I left --sra_only in there for backwards compatibility and added a deprecation notice in the help description for it.
  • Conda environment file
  • Dockerfile. Feel free to try out an image from my quay.io repo here.

Changed

  • Move everything into a main function to avoid variable shadowing
  • Provider is now optional and defaults to ena. I've also made the option case insensitive, rather than listing the cased versions of the available providers
  • If ENA download fails, try SRA
  • Reduced a bunch of execute calls which were mostly operations easily dealt with by pathlib.Path
  • Changed md5sum to use hashlib.md5 instead of executing a subprocess call to md5sum
  • Moved a bunch of import statements out of function bodies to the top of the file

Removed

  • Docstring at top of file as this is a mirror of argparse's help menu and creates needless maintenance

@rpetit3
Copy link
Owner

rpetit3 commented May 28, 2022

Awesome thank you very much @mbhall88 !

I've got to get a poster put together, but will get to this ASAP. Quick glance everything looked fine to me

Thanks again!

Copy link
Contributor Author

@mbhall88 mbhall88 left a comment

Choose a reason for hiding this comment

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

A very pedantic thought; maybe a minor version bump would be more suitable?

From semver

MINOR version when you add functionality in a backwards compatible manner

@rpetit3
Copy link
Owner

rpetit3 commented May 29, 2022

Good call!

@mbhall88
Copy link
Contributor Author

I also have a few other fixes incoming so maybe hold off merging

@mbhall88
Copy link
Contributor Author

Okay, I've updated the original comment with a list of all of the stuff I have done. Again, I won't be offended if you don't want some/all of this. I just thought I'd suggest it as I use this project quite heavily and find these things to be quite useful.

Thanks for creating this tool!

@rpetit3
Copy link
Owner

rpetit3 commented May 31, 2022

@mbhall88 let me know when you think its all set.

@mbhall88
Copy link
Contributor Author

mbhall88 commented Jun 1, 2022

I've successfully run it now on ~290 run accessions that wouldn't download successfully with v1.0.6 👍 So I reckon it's good to go.

@rpetit3
Copy link
Owner

rpetit3 commented Jun 1, 2022

Awesome! Just wanted to make sure you got all the changes in you'd like.

@rpetit3 rpetit3 merged commit 5e353ff into rpetit3:master Jun 1, 2022
@rpetit3
Copy link
Owner

rpetit3 commented Jun 1, 2022

Thank you so much @mbhall88 I'm planning a release for Bactopia and going to try and sneak this in there as well!

@mbhall88
Copy link
Contributor Author

mbhall88 commented Jun 1, 2022

Thank you for the SUPER helpful tool. I hate dealing with the SRA/ENA APIs so you've saved me a tonne of days dealing with that stuff.

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.

2 participants