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

update to openms 3.2.0 #423

Merged
merged 52 commits into from
Oct 7, 2024
Merged

update to openms 3.2.0 #423

merged 52 commits into from
Oct 7, 2024

Conversation

ypriverol
Copy link
Member

@ypriverol ypriverol commented Oct 2, 2024 •
edited by qodo-merge-pro bot
Loading

User description

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 pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/quantms branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

PR Type

enhancement, dependencies

Description

  • Updated GitHub Actions upload-artifact version from v1 to v4, and modified artifact names to include GitHub run ID.
  • Upgraded OpenMS third-party conda package version from 3.1.0 to 3.2.0 across multiple modules.
  • Upgraded quantms-utils conda package version from 0.0.10 to 0.0.11 across multiple modules.
  • Updated nextflow.config version from 1.3.0dev to 1.3.1dev.

Changes walkthrough 📝

Relevant files
Enhancement
1 files
ci.yml
Update GitHub Actions artifact version and naming               

.github/workflows/ci.yml

  • Updated GitHub Actions upload-artifact version from v1 to v4.
  • Modified artifact names to include GitHub run ID.
    		•  +6/-6     
    Dependencies
    14 files
    dev.config
    Upgrade OpenMS third-party package version                             

    conf/dev.config

  • Updated OpenMS third-party conda package version from 3.1.0 to 3.2.0.
    		•  +1/-1     
    main.nf
    Upgrade quantms-utils package version                                       

    modules/local/add_sage_feat/main.nf

    • Updated quantms-utils conda package version from 0.0.10 to 0.0.11.
    		 +3/-3     
    main.nf
    Upgrade quantms-utils package version                                       

    modules/local/diannconvert/main.nf

    • Updated quantms-utils conda package version from 0.0.10 to 0.0.11.
    		 +3/-3     
    main.nf
    Upgrade quantms-utils package version                                       

    modules/local/extract_psm/main.nf

    • Updated quantms-utils conda package version from 0.0.10 to 0.0.11.
    		 +3/-3     
    main.nf
    Upgrade quantms-utils package version                                       

    modules/local/extract_sample/main.nf

    • Updated quantms-utils conda package version from 0.0.10 to 0.0.11.
    		 +3/-3     
    main.nf
    Upgrade quantms-utils package version                                       

    modules/local/generate_diann_cfg/main.nf

    • Updated quantms-utils conda package version from 0.0.10 to 0.0.11.
    		 +3/-3     
    main.nf
    Upgrade quantms-utils package version                                       

    modules/local/ms2rescore/main.nf

    • Updated quantms-utils conda package version from 0.0.10 to 0.0.11.
    		 +3/-3     
    main.nf
    Upgrade quantms-utils package version                                       

    modules/local/mzmlstatistics/main.nf

    • Updated quantms-utils conda package version from 0.0.10 to 0.0.11.
    		 +3/-3     
    main.nf
    Upgrade OpenMS third-party package version                             

    modules/local/openms/consensusid/main.nf

  • Updated OpenMS third-party conda package version from 3.1.0 to 3.2.0.
    		•  +3/-3     
    main.nf
    Upgrade OpenMS third-party package version                             

    modules/local/openms/decoydatabase/main.nf

  • Updated OpenMS third-party conda package version from 3.1.0 to 3.2.0.
    		•  +3/-3     
    main.nf
    Upgrade OpenMS third-party package version                             

    modules/local/openms/epifany/main.nf

  • Updated OpenMS third-party conda package version from 3.1.0 to 3.2.0.
    		•  +3/-3     
    main.nf
    Upgrade OpenMS third-party package version                             

    modules/local/openms/extractpsmfeatures/main.nf

  • Updated OpenMS third-party conda package version from 3.1.0 to 3.2.0.
    		•  +3/-3     
    main.nf
    Upgrade OpenMS third-party package version                             

    modules/local/openms/falsediscoveryrate/main.nf

  • Updated OpenMS third-party conda package version from 3.1.0 to 3.2.0.
    		•  +3/-3     
    main.nf
    Upgrade OpenMS third-party package version                             

    modules/local/openms/filemerge/main.nf

  • Updated OpenMS third-party conda package version from 3.1.0 to 3.2.0.
    		•  +3/-3     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    ypriverol and others added 16 commits September 10, 2024 07:32
    @qodo-merge-pro Qodo Merge Pro
    Copy link

    qodo-merge-pro bot commented Oct 2, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Performance Improvement
    Added memory allocation parameters to the msgf_plus command, which may improve performance.

    Version Update
    Updated version from 1.3.0dev to 1.3.1dev. Ensure this change is reflected in other relevant files and documentation.

    @qodo-merge-pro Qodo Merge Pro
    Copy link

    qodo-merge-pro bot commented Oct 2, 2024 •
    edited
    Loading

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Update container image tags to match the new OpenMS version for better version control and reproducibility

    Consider updating the container image tag to match the new OpenMS version. The
    current container image tag is 'latest', which may not reflect the specific version
    3.2.0 of OpenMS that is now being used. It's generally a good practice to pin
    container versions to ensure reproducibility.

    conf/dev.config [20-22]

     withLabel: openms {
     conda = "openms::openms-thirdparty=3.2.0"
-    container = {"${ workflow.containerEngine == 'singularity' && !task.ext.singularity_pull_docker_container ? 'ghcr.io/openms/openms-executables-sif:latest' : 'ghcr.io/openms/openms-executables:latest' }"}
+    container = {"${ workflow.containerEngine == 'singularity' && !task.ext.singularity_pull_docker_container ? 'ghcr.io/openms/openms-executables-sif:3.2.0' : 'ghcr.io/openms/openms-executables:3.2.0' }"}
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion to pin the container image tag to the specific OpenMS version 3.2.0 enhances reproducibility and version control, aligning the container version with the updated conda package version. This is a best practice in software development.

    		9
    Update the version number to reflect the significance of the OpenMS version change

    Consider updating the version number to reflect a more significant change, given
    that you've updated to a new minor version of OpenMS (from 3.1.0 to 3.2.0).
    Typically, when updating dependencies to a new minor version, it's appropriate to
    increment your own minor version number.

    nextflow.config [495]

    -version         = '1.3.1dev'
+version         = '1.4.0dev'
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: While updating the version number to 1.4.0dev could better reflect the significance of the dependency update, the current change to 1.3.1dev is not incorrect. The suggestion is valid but not critical, as versioning can be subjective and context-dependent.

    		7
    Maintainability
    Use environment variables for version numbers to simplify future updates

    Consider using environment variables for version numbers to make future updates
    easier. This allows you to change the version in one place instead of multiple
    places throughout the file.

    .github/workflows/ci.yml [108]

    -- uses: actions/upload-artifact@v4
+- uses: actions/upload-artifact@${{ env.UPLOAD_ARTIFACT_VERSION }}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using environment variables for version numbers is a good practice that simplifies future updates and reduces the risk of errors. This suggestion is relevant and can enhance maintainability by centralizing version control.

    		7
    Use more descriptive variable names to improve code readability

    Consider using a more descriptive variable name instead of 'task.memory'. For
    example, 'availableMemory' or 'allocatedMemory' would provide more context about
    what the memory value represents.

    modules/local/openms/thirdparty/msgfdb_indexing/main.nf [23-26]

     msgf_plus edu.ucsd.msjava.msdbsearch.BuildSA \
     -d ${database} \
-    -Xmx${task.memory.toMega()}m \
-    -Xms${task.memory.toMega()}m \
+    -Xmx${availableMemory.toMega()}m \
+    -Xms${availableMemory.toMega()}m \
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to use a more descriptive variable name like 'availableMemory' instead of 'task.memory' can improve code readability and maintainability. However, it requires ensuring that the new variable name is defined and used consistently throughout the codebase.

    		5

    💡 Need additional feedback ? start a PR chat

    @github-actions GitHub Actions
    Copy link

    github-actions bot commented Oct 2, 2024 •
    edited
    Loading

    nf-core lint overall result: Passed ✅

    Posted for pipeline commit 9d47118

    +| ✅ 278 tests passed       |+
#| ❔  10 tests were ignored |#

    ❔ Tests ignored:

    • files_exist - File is ignored: conf/igenomes.config
    • files_exist - File is ignored: conf/test_full.config
    • files_exist - File is ignored: conf/test.config
    • files_exist - File is ignored: .github/workflows/awstest.yml
    • files_exist - File is ignored: .github/workflows/awsfulltest.yml
    • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
    • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
    • files_unchanged - File ignored due to lint config: docs/README.md
    • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/quantms/quantms/.github/workflows/awstest.yml
    • multiqc_config - multiqc_config

    ✅ Tests passed:

    Run details

    • nf-core/tools version 2.14.1
    • Run at 2024-10-07 13:26:05

    @ypriverol ypriverol requested a review from jpfeuffer October 2, 2024 14:49
    @jpfeuffer
    Copy link
    Collaborator

    jpfeuffer commented Oct 2, 2024 •
    edited
    Loading

    @timosachsenberg PR OpenMS/OpenMS#7326 apparently broke the workflow. Any idea?
    I checked. We are not overwriting any correction factor. Defaults should be used. Apparently they do not work anymore.

    Command error:
  Progress of 'loading mzML':
    Progress of 'loading spectra list':
  
      7.31 %               
      75.87 %               
    -- done [took 2.52 s (CPU), 1.46 s (Wall)] -- 
    Progress of 'loading chromatogram list':
  
    -- done [took 0.00 s (CPU), 0.00 s (Wall)] -- 
  
  -- done [took 2.53 s (CPU), 1.46 s (Wall) @ 51.06 MiB/s] -- 
  Selecting scans with activation mode: auto
  Filtering by MS/MS(/MS) and activation mode:
    level 2: 6103 scans
  Using MS-level 2 for quantification.
  Calibration stats: Median distance of observed reporter ions m/z to expected position (up to 0.5 Th):
    ch 126  (~126.127): -0.00104392 Th
    ch 127  (~127.124): -0.00730482 Th
    ch 128  (~128.134): -0.00111082 Th
    ch 129  (~129.131): -0.00728017 Th
    ch 130  (~130.141): -0.00111669 Th
    ch 131  (~131.138): -0.000999415 Th
  
  Invalid parameter: IsobaricIsotopeCorrector: The given isotope correction matrix is an identity matrix leading to no correction. Please provide a valid isotope_correction matrix as it was provided with the sample kit!

    @timosachsenberg
    Copy link

    @cbielow you have a deeper understanding of that code. Any idea?

    @jpfeuffer
    Copy link
    Collaborator

    jpfeuffer commented Oct 2, 2024 •
    edited
    Loading

    I am guessing that isobaricanalyzer was "broken" in general after that PR and only works in our tests because we completely supply an ini file with prefilled correction matrix.
    Probably taking the default is not working or not supported anymore.

    @ypriverol
    Copy link
    Member Author

    @jpfeuffer @timosachsenberg @cbielow Taking the default will not work because that option do not exist anymore by reading this https://github.com/OpenMS/OpenMS/blob/ff325fe8572c953a6dbad3eb14b7facebf5549e5/src/topp/IsobaricAnalyzer.cpp#L95 looks like we have to provide for every method the matrix (then the concept of default is gone). Where and how I can generate what it used to be default metrics/values?

    @ypriverol ypriverol linked an issue Oct 5, 2024 that may be closed by this pull request
    Update openMS 3.2.0 #424
    Closed
    @ypriverol ypriverol linked an issue Oct 5, 2024 that may be closed by this pull request
    image of the quantms workflow still refers to nf-core and version 1.2 #417
    Closed
    @jpfeuffer
    Copy link
    Collaborator

    Does anyone know why it does not find the .nextflow.log anymore? Do we log to a different directory? Did nextflow change the default location?

    @ypriverol
    Copy link
    Member Author

    Does anyone know why it does not find the .nextflow.log anymore? Do we log to a different directory? Did nextflow change the default location?

    Well spot, let me find out.

    @ypriverol
    Copy link
    Member Author

    Does anyone know why it does not find the .nextflow.log anymore? Do we log to a different directory? Did nextflow change the default location?

    Solved.

    jpfeuffer
    jpfeuffer reviewed Oct 7, 2024
    View reviewed changes
    .github/workflows/ci.yml Outdated
    @@ -81,7 +82,7 @@ jobs:
    # For example: adding multiple test runs with different parameters
    # Remember that you can parallelise this by using strategy.matrix
    run: |
    nextflow run ${GITHUB_WORKSPACE} -profile $TEST_PROFILE,$EXEC_PROFILE --outdir ${TEST_PROFILE}_${EXEC_PROFILE}_results
    nextflow -log pipeline.log run ${GITHUB_WORKSPACE} -profile $TEST_PROFILE,$EXEC_PROFILE --outdir ${TEST_PROFILE}_${EXEC_PROFILE}_results
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    Why change the log filename? This just confused everyone.

    Copy link
    Member Author

    @ypriverol ypriverol Oct 7, 2024 •
    edited
    Loading

    Choose a reason for hiding this comment

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

    I think is the plugin that we use to run the pipeline. I think it deletes or do something with the .nextflow.log. In a previous run I checked all the files created in the run and they were not .nextflow.log, check this https://github.com/bigbio/quantms/actions/runs/11211083164/job/31159229622

    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    But who added this line? And why? It is not needed and broke the additional CI log uploads that I implemented.

    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    Of course there is no .nextflow.log anymore if you overwrite the default here.

    Copy link
    Member Author

    Choose a reason for hiding this comment

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

    Of course there is no .nextflow.log anymore if you overwrite the default here.

    Before writing the -log option it was not there. Check my CI/CD before adding the -log. I did try to ls all the files but the .nextflow.log was not there already.

    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    Which plugin do you mean?

    Copy link
    Member Author

    Choose a reason for hiding this comment

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

    Sorry misunderstood. I don't know why .nextflow.log is not there. I did the ls -R -a, but the file is not there, my point is that in other nf-core pipelines, it is not there.

    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    I'm not sure if it is not there in other pipelines. They just don't try to upload it.
    They are clearly saying it exists in the docs:
    https://nf-co.re/docs/usage/troubleshooting/basics#categorize-the-type-of-error

    @jpfeuffer
    Copy link
    Collaborator

    The problem is that your ls -la does not show hidden files..
    We would have found out, that the file is actually there and that the upload-action@v4 has a new parameter include-hidden-files.

    @ypriverol ypriverol merged commit b7c9b6a into bigbio:master Oct 7, 2024
    23 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @jpfeuffer jpfeuffer jpfeuffer approved these changes

    @timosachsenberg timosachsenberg Awaiting requested review from timosachsenberg

    Assignees
    No one assigned
    Labels
    dependencies enhancement New feature or request Review effort [1-5]: 3
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    Update openMS 3.2.0 image of the quantms workflow still refers to nf-core and version 1.2
    5 participants
    @ypriverol @jpfeuffer @timosachsenberg @cbielow @daichengxin