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 merge conflicts with v1.13.2 template #74

Merged
merged 15 commits into from
May 6, 2021
Merged

Fix merge conflicts with v1.13.2 template #74

merged 15 commits into from
May 6, 2021

Conversation

ErikDanielsson
Copy link
Contributor

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 necessary, also make a PR on the nf-core/smrnaseq branch on the nf-core/test-datasets repo
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Make sure your code lints (nf-core lint .).
  • Documentation in docs is updated
  • CHANGELOG.md is updated
  • README.md is updated

Learn more about contributing: https://github.com/nf-core/smrnaseq/tree/master/.github/CONTRIBUTING.md

@ewels
Copy link
Member

ewels commented Apr 13, 2021

Not quite sure what's going on with the linting logs - tonnes of sqlite log messages being spat out 👀 Do you see this locally as well @ErikDanielsson ?

@ewels
Copy link
Member

ewels commented Apr 13, 2021

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 1ab5a5c

+| ✅ 159 tests passed       |+
!| ❗  24 tests had warnings |!

❗ Test warnings:

  • actions_awsfulltest - .github/workflows/awsfulltest.yml should test full datasets, not -profile test
  • conda_env_yaml - Conda dep outdated: conda-forge::r-base=3.6.3, 4.0.3 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-statmod=1.4.32, 1.4.35 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-data.table=1.12.4, 1.14.0 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-gplots=3.0.1.1, 3.1.1 available
  • conda_env_yaml - Conda dep outdated: conda-forge::r-r.methodss3=1.7.1, 1.8.1 available
  • conda_env_yaml - Conda dep outdated: conda-forge::python=3.7.3, 3.9.2 available
  • conda_env_yaml - Conda dep outdated: conda-forge::markdown=3.1.1, 3.3.4 available
  • conda_env_yaml - Conda dep outdated: conda-forge::pymdown-extensions=6.0, 8.1.1 available
  • conda_env_yaml - Conda dep outdated: conda-forge::pygments=2.6.1, 2.8.1 available
  • conda_env_yaml - Conda dep outdated: conda-forge::pigz=2.3.4, 2.6 available
  • conda_env_yaml - Conda dep outdated: bioconda::trim-galore=0.6.5, 0.6.6 available
  • conda_env_yaml - Conda dep outdated: bioconda::samtools=1.9, 1.12 available
  • conda_env_yaml - Conda dep outdated: bioconda::bowtie=1.2.3, 1.3.0 available
  • conda_env_yaml - Conda dep outdated: bioconda::multiqc=1.9, 1.10.1 available
  • conda_env_yaml - Conda dep outdated: bioconda::htseq=0.11.3, 0.13.5 available
  • conda_env_yaml - Conda dep outdated: bioconda::seqkit=0.12.0, 0.15.0 available
  • conda_env_yaml - Conda dep outdated: bioconda::bioconductor-edger=3.26.5, 3.32.1 available
  • conda_env_yaml - Conda dep outdated: bioconda::bioconductor-limma=3.40.2, 3.46.0 available
  • pipeline_todos - TODO string in README.md: Add bibliography of tools and data used in your pipeline
  • pipeline_todos - TODO string in nextflow.config: Specify your pipeline's command line flags
  • pipeline_todos - TODO string in awstest.yml: You can customise CI pipeline run tests as required
  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required
  • schema_params - Schema param config_profile_name not found from nextflow config

✅ Tests passed:

Run details

  • nf-core/tools version 1.13.3
  • Run at 2021-04-13 08:50:21

@ewels
Copy link
Member

ewels commented Apr 13, 2021

Looks like you have a few linting warnings that can be cleaned up still ☝🏻 The pipeline_todos and schema_params ones might warrant some attention (some stuff that hasn't been done can be left, eg. the aws full test stuff).

@ErikDanielsson
Copy link
Contributor Author

I get no log messages when running the linting locally, oddly enough, @ewels.

@ewels
Copy link
Member

ewels commented Apr 13, 2021

I get no log messages when running the linting locally, oddly enough, @ewels.

I suspect that this is due to a change in a dependency then - this stuff sometimes appears on GitHub Actions first as it does a totally fresh install every time so uses the latest versions of everything.

If we download the GHA artefact with the verbose log it might say which python module these messages are coming from..

@ewels
Copy link
Member

ewels commented Apr 13, 2021

Ah yes, our friend requests_cache doing loads of log.info() calls. Fantastic.

If you fancy a slightly more tricky / python task, a PR to nf-core/tools to downgrade these messages to debug would be fantastic.

Should be something like this (see elsewhere):

# Only show error messages from pipeline creation
logging.getLogger("requests_cache").setLevel(logging.WARNING)

@ErikDanielsson
Copy link
Contributor Author

Sounds good, I'll give that a go!

@ewels
Copy link
Member

ewels commented Apr 13, 2021

Ah yes, our friend requests_cache doing loads of log.info() calls. Fantastic.

Next time we're on a call, remind me to show you how I figured this out...

@ErikDanielsson
Copy link
Contributor Author

I noticed that the schema param config_profile_name which is causing a lint warning is missing from the template nextflow.config as well, perhaps this test should warn with the current template?

@ewels
Copy link
Member

ewels commented Apr 13, 2021

config_profile_name

Hmm I'm lost by this. x-ref https://github.com/nf-core/tools/pull/892/files . @KevinMenden do you know what should be going on here?

@KevinMenden
Copy link
Contributor

Nope, no idea .... config_profile_name is nowhere to be found in the nextflow.config or main.nf .... so I'm wondering if we can just leave it out? 🤔

@KevinMenden
Copy link
Contributor

Ah, you added that param about a month ago 🤔 okay I'm really lost here 🤷

@JWCook
Copy link

JWCook commented Apr 13, 2021

Ah yes, our friend requests_cache doing loads of log.info() calls. Fantastic.

Those have been bumped down to debug level in requests-cache 0.6.1. You should have less spammy logs now.

@ewels
Copy link
Member

ewels commented Apr 13, 2021

Ah yes, our friend requests_cache doing loads of log.info() calls. Fantastic.

Those have been bumped down to debug level in requests-cache 0.6.1. You should have less spammy logs now.

Eagle eyes @JWCook! :octocat: 👀 That's great to hear, thanks for letting us know 👍🏻 Ok then I don't think any action is needed.. Even better 🎉

@ewels
Copy link
Member

ewels commented Apr 13, 2021

I noticed that the schema param config_profile_name which is causing a lint warning is missing from the template nextflow.config as well, perhaps this test should warn with the current template?

@ErikDanielsson anyway yes, I think that config_profile_name should be in both the schema and nextflow.config 👍🏻

I think that it also needs adding to the nextflow.config file in the template itself: https://github.com/nf-core/tools/blob/ee90d06f4e09c395c913a50b4266dd12fe7f179b/nf_core/pipeline-template/nextflow.config#L34 (as you spotted).

No change to the lint tests is needed.

@KevinMenden
Copy link
Contributor

@ErikDanielsson are you still working on this?
Need to fix some bugs with the smRNA-seq pipeline, would be nice to merge this before 😁

I'll go through the PR either today or tomorrow to see if I can help.

Copy link
Contributor

@KevinMenden KevinMenden left a comment

Choose a reason for hiding this comment

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

Alright this looks good to me, not sure why the tests are not running though 🤔

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Thanks @ErikDanielsson for this, and thanks @KevinMenden for pushing it over the finishing line..

@ewels ewels merged commit 3a2044e into nf-core:dev May 6, 2021
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.

6 participants