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

Using packaging.version instead of pkg_resources #2825

Closed
wants to merge 6 commits into from
Closed

Using packaging.version instead of pkg_resources #2825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4f099fc
Using packaging.version instead of pkg_resources
asp8200 Mar 1, 2024
ae59ec0
[automated] Update CHANGELOG.md
nf-core-bot Mar 1, 2024
be43511
fix type
mashehu Mar 1, 2024
a674310
Merge branch 'dev' into 1984_avoid_importing_pkg_resources
asp8200 Mar 15, 2024
d70671e
Merge branch 'dev' into 1984_avoid_importing_pkg_resources
asp8200 Mar 15, 2024
3a05c73
Merge branch 'dev' into 1984_avoid_importing_pkg_resources
tomiles Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
- Update CI to use nf-core/setup-nextflow v2
- Changelog bot: handle also patch version before dev suffix ([#2820](https://github.com/nf-core/tools/pull/2820))
- Fix path in component update script ([#2823](https://github.com/nf-core/tools/pull/2823))
- Using packaging.version instead of pkg_resources ([#2825](https://github.com/nf-core/tools/pull/2825))
- Update prettier to 3.2.5 ([#2830](https://github.com/nf-core/tools/pull/2830))
- Update GitHub Actions ([#2827](https://github.com/nf-core/tools/pull/2827))
- Switch to setup-nf-test ([#2834](https://github.com/nf-core/tools/pull/2834))
Expand Down
6 changes: 2 additions & 4 deletions nf_core/download.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import rich
import rich.progress
from git.exc import GitCommandError, InvalidGitRepositoryError
from pkg_resources import parse_version as version_parser
from packaging.version import Version

import nf_core
import nf_core.list
Expand Down Expand Up @@ -1664,9 +1664,7 @@ def tidy_tags_and_branches(self):
else:
# desired revisions may contain arbitrary branch names that do not correspond to valid sematic versioning patterns.
valid_versions = [
version_parser(v)
for v in desired_revisions
if re.match(r"\d+\.\d+(?:\.\d+)*(?:[\w\-_])*", v)
str(Version(v)) for v in desired_revisions if re.match(r"\d+\.\d+(?:\.\d+)*(?:[\w\-_])*", v)
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with the usage of packaging.Version() as it stands here now is mentioned in the comment above.

desired revisions may contain arbitrary branch names that do not correspond to valid semantic versioning patterns.

And packaging.Version has its own definition of what a valid version identifier is and it is not completely compatible with semantic versions either.

As an example:

>>> [str(Version(v)) for v in ["1.1.1-latest"] if re.match(r"\d+\.\d+(?:\.\d+)*(?:[\w\-_])*", v)]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 1, in <listcomp>
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/site-packages/packaging/version.py", line 298, in __init__
    raise InvalidVersion("Invalid version: '{0}'".format(version))
packaging.version.InvalidVersion: Invalid version: '1.1.1-latest'

This will not work nicely if any of the desired revisions fails validity checking inside Version().

The previous version parser from pkg_resources import parse_version returns a LegacyVersion if it can't parse the identifier, which makes it much better suited in this context than packaging.Version.

>>> [version_parser(v) for v in ["1.1.1", "1.2.3-latest"]]
[<Version('1.1.1')>, <LegacyVersion('1.2.3-latest')>]

I am not versed enough to know what kind of strings can appear in desired_revisions but I believe this code should be able to sort a broader range of version specifiers.

Copy link
Contributor

Choose a reason for hiding this comment

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

A separate issue is the conversion of the Version instance to a string. It will lead to wrong orderings as demonstrated below with a version number which had a release candidate.

>>> sorted(str(Version(v)) for v in ['1.23.4', '1.23.4-1', '1.23.5', '1.23.5rc'])[-1]
'1.23.5rc0'
>>> sorted(Version(v) for v in ['1.23.4', '1.23.4-1', '1.23.5', '1.23.5rc'])[-1] 
<Version('1.23.5')>

]
# valid versions sorted in ascending order, last will be aliased as "latest".
latest = sorted(valid_versions)[-1]
Expand Down
Loading