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.

Sign up for GitHub

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

Jump to bottom

Replace base path for test data with new form #4723

Merged
merged 8 commits into from
Jan 12, 2024
Merged

Replace base path for test data with new form #4723

merged 8 commits into from
Jan 12, 2024

Conversation

adamrtalbot
Copy link
Contributor

Changes:

  • Base path for test data is now replaced with a more logical form based on discussions on Slack
  • from modules_test_data_base to testdata_base_path
  • Should be more explicit and reduce confusion with 'database'

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • PROFILE=docker pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
    • PROFILE=singularity pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware
    • PROFILE=conda pytest --tag <MODULE> --symlink --keep-workflow-wd --git-aware

@adamrtalbot
Replace base path for test data with new form
17cfaac 
Changes:
 - Base path for test data is now replaced with a more logical form based on discussions on Slack
 - from modules_test_data_base to testdata_base_path
 - Should be more explicit and reduce confusion with 'database'
@maxulysse
Copy link
Member

Can you wait 5 min, and we test with s3?

@adamrtalbot
Copy link
Contributor Author

Can you wait 5 min, and we test with s3?

yep.

@adamrtalbot
Copy link
Contributor Author

Let's make sure it all works before using S3.

@adamrtalbot adamrtalbot marked this pull request as draft January 11, 2024 10:44
maxulysse
maxulysse reviewed Jan 11, 2024
View reviewed changes
modules/nf-core/sratools/fasterqdump/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/sratools/fasterqdump/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/sratools/fasterqdump/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/sratools/fasterqdump/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/untar/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/untar/tests/main.nf.test Outdated Show resolved Hide resolved
tests/config/nf-test.config Outdated Show resolved Hide resolved
@adamrtalbot @maxulysse
Update to use s3 base path
1adab2a 
Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>
maxulysse
maxulysse approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@maxulysse maxulysse left a comment

Choose a reason for hiding this comment

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

LGTM

@adamrtalbot adamrtalbot marked this pull request as ready for review January 11, 2024 11:36
@maxulysse maxulysse marked this pull request as draft January 11, 2024 12:16
@maxulysse
Copy link
Member

Keeping it as a draft for now

@adamrtalbot adamrtalbot marked this pull request as ready for review January 11, 2024 15:54
maxulysse
maxulysse reviewed Jan 11, 2024
View reviewed changes
tests/config/nf-test.config
@@ -2,7 +2,7 @@ params {
publish_dir_mode = "copy"
singularity_pull_docker_container = false
test_data_base = 'https://raw.githubusercontent.com/nf-core/test-datasets/modules'
modules_test_data_base = 'https://raw.githubusercontent.com/nf-core/test-datasets/3e79d4b5fbed8c6ef7546d50696b343ab9a95fbe/'
testdata_base_path = 's3://ngi-igenomes/testdata/nf-core/modules/'
Copy link
Member

Choose a reason for hiding this comment

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

Actually if we want to use files from github, we can use

modules_test_data_base = 'https://raw.githubusercontent.com/nf-core/test-datasets/modules/data/

and we're good, so we still are backward compatible-ish.

We could even update https://github.com/nf-core/modules/blob/master/tests/config/test_data.config so that data is in test_data_base and not in the paths, and we can easily switch from s3 to github

Copy link
Member

Choose a reason for hiding this comment

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

nevermind actually, since we plan to drop the usage of this file, we actually don't need anything done

Copy link
Member

Choose a reason for hiding this comment

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

just modules_test_data_base = 'https://raw.githubusercontent.com/nf-core/test-datasets/modules/data/' and we're good to go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, do we want to pull from GH by default?

Copy link
Member

Choose a reason for hiding this comment

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

no, but it's still a possibility, that all I'm saying

Copy link
Member

Choose a reason for hiding this comment

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

What we achieve here is tests portability, and we proved that by mirroring all we have on GitHub to s3.
Now one could test with s3, github links or local path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, merge away?

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a ✔️ from @drpatelh?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We spend our lives seeking @drpatelh's approval. It's like Succession with API failures.

Copy link
Member

Choose a reason for hiding this comment

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

so I did test with local path, and it works

@adamrtalbot adamrtalbot mentioned this pull request Jan 12, 2024
Merged
16 tasks
@adamrtalbot adamrtalbot mentioned this pull request Jan 12, 2024
Closed
14 tasks
@drpatelh drpatelh added this pull request to the merge queue Jan 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 12, 2024
@adamrtalbot adamrtalbot added this pull request to the merge queue Jan 12, 2024
Merged via the queue into nf-core:master with commit 20e78a9 Jan 12, 2024
31 checks passed
@adamrtalbot adamrtalbot deleted the replace_base_path_for_test_data branch January 12, 2024 14:03
@drpatelh drpatelh mentioned this pull request Jan 12, 2024
Closed
lrauschning pushed a commit to lrauschning/modules that referenced this pull request Jan 17, 2024
@adamrtalbot @maxulysse @lrauschning
Replace base path for test data with new form (nf-core#4723)
0343668 
* Replace base path for test data with new form

Changes:
 - Base path for test data is now replaced with a more logical form based on discussions on Slack
 - from modules_test_data_base to testdata_base_path
 - Should be more explicit and reduce confusion with 'database'

* Update to use s3 base path

Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>

* Remove date path

* Remove leading slash from file path and move to parameter value

* sratools/prefetch uses modules_test_data_base_path

* Update md5

* Removed variable md5sum

---------

Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>
lrauschning pushed a commit to lrauschning/modules that referenced this pull request Jan 17, 2024
@adamrtalbot @maxulysse @lrauschning
Replace base path for test data with new form (nf-core#4723)
5961695 
* Replace base path for test data with new form

Changes:
 - Base path for test data is now replaced with a more logical form based on discussions on Slack
 - from modules_test_data_base to testdata_base_path
 - Should be more explicit and reduce confusion with 'database'

* Update to use s3 base path

Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>

* Remove date path

* Remove leading slash from file path and move to parameter value

* sratools/prefetch uses modules_test_data_base_path

* Update md5

* Removed variable md5sum

---------

Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>
jennylsmith pushed a commit to RSC-RP/modules that referenced this pull request Mar 20, 2024
@adamrtalbot @maxulysse @jennylsmith
Replace base path for test data with new form (nf-core#4723)
f2b3240 
* Replace base path for test data with new form

Changes:
 - Base path for test data is now replaced with a more logical form based on discussions on Slack
 - from modules_test_data_base to testdata_base_path
 - Should be more explicit and reduce confusion with 'database'

* Update to use s3 base path

Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>

* Remove date path

* Remove leading slash from file path and move to parameter value

* sratools/prefetch uses modules_test_data_base_path

* Update md5

* Removed variable md5sum

---------

Co-authored-by: Maxime U Garcia <maxime.garcia@seqera.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxulysse maxulysse maxulysse approved these changes

@JoseEspinosa JoseEspinosa Awaiting requested review from JoseEspinosa

@drpatelh drpatelh Awaiting requested review from drpatelh

@matthdsm matthdsm Awaiting requested review from matthdsm

@jfy133 jfy133 Awaiting requested review from jfy133

@Midnighter Midnighter Awaiting requested review from Midnighter

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adamrtalbot @maxulysse