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

Do not remove invalid software versions #2166

Merged
merged 6 commits into from Nov 9, 2023
Merged

Conversation

ewels
Copy link
Member

@ewels ewels commented Nov 9, 2023

Added a bit of logic to clean up version strings, and also catch exceptions coming from packaging.version.parse:

  /// MultiQC 🔍 | v1.18.dev0 (c62a430)

|           multiqc | Search path : /Users/ewels/Downloads/multiqc_input
|         searching | ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 84/84
|    custom_content | seqeralabs-nf-aggregate-summary: Found 1 sample (html)
|        seqera_cli | Found 9 reports
Traceback (most recent call last):
  File "/Users/ewels/.miniconda3/miniconda3/envs/py3.10/bin/multiqc", line 8, in <module>
    sys.exit(run_multiqc())
  File "/Users/ewels/GitHub/MultiQC/MultiQC/multiqc/__main__.py", line 23, in run_multiqc
    multiqc.run_cli(prog_name="multiqc")
  File "/Users/ewels/.miniconda3/miniconda3/envs/py3.10/lib/python3.10/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
  File "/Users/ewels/.miniconda3/miniconda3/envs/py3.10/lib/python3.10/site-packages/rich_click/rich_command.py", line 126, in main
    rv = self.invoke(ctx)
  File "/Users/ewels/.miniconda3/miniconda3/envs/py3.10/lib/python3.10/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/ewels/.miniconda3/miniconda3/envs/py3.10/lib/python3.10/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/Users/ewels/GitHub/MultiQC/MultiQC/multiqc/multiqc.py", line 300, in run_cli
    multiqc_run = run(**kwargs)
  File "/Users/ewels/GitHub/MultiQC/MultiQC/multiqc/multiqc.py", line 804, in run
    software_versions.update_versions_from_config(config, report)
  File "/Users/ewels/GitHub/MultiQC/MultiQC/multiqc/utils/software_versions.py", line 28, in update_versions_from_config
    versions_from_config = load_versions_from_config(config)
  File "/Users/ewels/GitHub/MultiQC/MultiQC/multiqc/utils/software_versions.py", line 87, in load_versions_from_config
    software_versions_file_tmp, is_valid = validate_software_versions(software_versions_file_tmp)
  File "/Users/ewels/GitHub/MultiQC/MultiQC/multiqc/utils/software_versions.py", line 169, in validate_software_versions
    if not _list_of_versions_is_good(versions):
  File "/Users/ewels/GitHub/MultiQC/MultiQC/multiqc/utils/software_versions.py", line 150, in _list_of_versions_is_good
    elif not packaging.version.parse(item):
  File "/Users/ewels/.miniconda3/miniconda3/envs/py3.10/lib/python3.10/site-packages/packaging/version.py", line 54, in parse
    return Version(version)
  File "/Users/ewels/.miniconda3/miniconda3/envs/py3.10/lib/python3.10/site-packages/packaging/version.py", line 200, in __init__
    raise InvalidVersion(f"Invalid version: '{version}'")
packaging.version.InvalidVersion: Invalid version: '0.9.0 (build 0cea70d)'

The regex in this PR normalises 0.9.0 (build 0cea70d) to 0.9.0, fixing the issue.

But - honestly, I'm not really sure if we want to do this.

Why do we have to validate that version strings look like version strings? Can we not just accept any string?

Comments like this make me think that the code did that at some point:

Accepts list with both strings and packaging.version.Version objects.

However, as far as I see it, if the version string isn't classic semver / recognised format, it's entirely excluded from the report:

| software_versions | Invalid version: '0.9.0 (build 0cea70d)'
| software_versions | Software versions loaded from ./11/software_mqc_versions.yml is not in a valid format, ignoring the file.

Worse - not just that version string, but all versions within that file.

Tagging @vladsavelyev @pontushojer

@vladsavelyev
Copy link
Member

I agree that it doesn't make sense to attempt to parse versions, any string should be allowed 👍

Pushed a commit.

@vladsavelyev vladsavelyev self-requested a review November 9, 2023 16:16
@vladsavelyev vladsavelyev changed the title Clean version strings with build IDs Do not remove invalid software versions Nov 9, 2023
@vladsavelyev vladsavelyev merged commit 5832ba7 into master Nov 9, 2023
6 checks passed
@ewels ewels deleted the clean-version-strings branch December 18, 2023 11:55
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.

None yet

3 participants