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 library-wise logic to atlas-fastq-provider wrapper #262

Merged
merged 25 commits into from
Oct 13, 2022

Conversation

pmb59
Copy link
Member

@pmb59 pmb59 commented Mar 10, 2022

Description

Make some refinements to the atlas-fastq-provider galaxy wrapper. ISL uses the library-wise downloader rather than file-wise, we need an extra option in the wrapper for that.

Basically, the wrapper needs a library-wise logic that calls fetchEnaLibraryFastqs.sh (if library-name param is provided) or fetchFastq.sh (if filename param is provided). Depending on each case, specific set of parameters need to be displayed.

This PR is related to ebi-gene-expression-group/atlas-fastq-provider#17


Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • I have made any required changes to upstream dependencies for a tool wrapper, and they are available in distribution channels (e.g. Pip, Conda).
  • If I have updated the underlying software for a tool wrapper (e.g. scanpy-scripts by changing the value of @TOOL_VERSION@), then I have reset all 'build' values to 0 (e.g. @TOOL_VERSION@+galaxy0)
  • If I have updated a tool wrapper without a software change, then I have bumped the associated 'build' values (e.g. @TOOL_VERSION@+galaxy0 @TOOL_VERSION@+galaxy1)

@pmb59 pmb59 changed the title Add library-wise mode to atlas-fastq-provider [WIP] Add library-wise mode to atlas-fastq-provider Mar 10, 2022
@pmb59 pmb59 changed the title [WIP] Add library-wise mode to atlas-fastq-provider [WIP] Add library-wise logic to atlas-fastq-provider wrapper Mar 15, 2022
@pmb59 pmb59 changed the title [WIP] Add library-wise logic to atlas-fastq-provider wrapper Add library-wise logic to atlas-fastq-provider wrapper Jun 9, 2022
@pmb59 pmb59 requested a review from pcm32 June 16, 2022 08:51
@pmb59
Copy link
Member Author

pmb59 commented Jun 16, 2022

@pcm32 We need to bump atlas-fastq-provider to 0.4.3 and deploy in a new conda version before this will work.

Also we might need to add:

  • a conditional to the outputs based on main_source (lib or file)
  • a new test for main_source="library"

@pcm32
Copy link
Member

pcm32 commented Jun 20, 2022

I guess we are waiting for the bump here, correct?

Copy link
Member

@pcm32 pcm32 left a comment

Choose a reason for hiding this comment

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

Some comments to improve this.

tools/fastq_provider/fastq_provider.xml Outdated Show resolved Hide resolved
tools/fastq_provider/fastq_provider.xml Outdated Show resolved Hide resolved
tools/fastq_provider/fastq_provider.xml Outdated Show resolved Hide resolved
tools/fastq_provider/fastq_provider.xml Outdated Show resolved Hide resolved
tools/fastq_provider/fastq_provider.xml Outdated Show resolved Hide resolved
@irisdianauy irisdianauy marked this pull request as draft October 5, 2022 03:07
@irisdianauy irisdianauy changed the title Add library-wise logic to atlas-fastq-provider wrapper [WIP] Add library-wise logic to atlas-fastq-provider wrapper Oct 5, 2022
@irisdianauy irisdianauy marked this pull request as ready for review October 6, 2022 09:27
@irisdianauy irisdianauy changed the title [WIP] Add library-wise logic to atlas-fastq-provider wrapper Add library-wise logic to atlas-fastq-provider wrapper Oct 6, 2022
@irisdianauy irisdianauy requested a review from pcm32 October 6, 2022 09:44
@pmb59 pmb59 requested a review from anilthanki October 7, 2022 09:20
@anilthanki
Copy link
Collaborator

@pmb59 and @irisdianauy can you please add couple of tests utilising Resources and directory and Retrieval Method options.

Copy link
Member

@pcm32 pcm32 left a comment

Choose a reason for hiding this comment

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

Some minor comments

tools/fastq_provider/fastq_provider.xml Outdated Show resolved Hide resolved
tools/fastq_provider/fastq_provider.xml Outdated Show resolved Hide resolved
Copy link
Collaborator

@anilthanki anilthanki left a comment

Choose a reason for hiding this comment

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

LGTM

@pmb59
Copy link
Member Author

pmb59 commented Oct 13, 2022

Thanks @irisdianauy and @anilthanki As all requests by @pcm32 are done, we are ready to merge.

@pmb59 pmb59 merged commit a8c99cb into develop Oct 13, 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.

5 participants