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

Allow some mutually exclusive same name deps #234

Closed
wants to merge 3 commits into from

Conversation

BeRT2me
Copy link

@BeRT2me BeRT2me commented Sep 27, 2023

Given a pyproject.toml that contains:

[[tool.poetry.source]]
name = "torch"
url = "https://download.pytorch.org/whl/cpu/"
priority = "explicit"

[tool.poetry.dependencies]
python = "~3.10"
torch = [
     {version = "^2.0.1", platform = "darwin", source = "PyPI"},
     {version = "^2.0.1+cpu", platform = "linux", source = "torch"}
]

Running poetry export produces:

torch==2.0.1+cpu ; python_version >= "3.10" and python_version < "3.11" and (sys_platform == "darwin" or sys_platform == "linux")

Which is strictly wrong. Attempting to install this on a darwin system will fail.

By stripping the package version of the "local" portion (+cpu), then due to how they're ordered by default in the poetry.lock file, both dependencies are able to be correctly included.

With this change, poetry export correctly produces:

torch==2.0.1 ; python_version >= "3.10" and python_version < "3.11" and sys_platform == "darwin"
torch==2.0.1+cpu ; python_version >= "3.10" and python_version < "3.11" and sys_platform == "linux"

P.S.
I also think things look better with the built-in textwrap.dedent to keep relevant text in-line rather than to the left.

MARKER_PY27_PY36_ONLY was added due to line-length issues.

@BeRT2me
Copy link
Author

BeRT2me commented Sep 27, 2023

@sonarcloud
Copy link

sonarcloud bot commented Sep 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@@ -118,7 +118,7 @@ def get_project_dependencies(
# Put higher versions first so that we prefer them.
for packages in packages_by_name.values():
packages.sort(
key=lambda package: package.version,
key=lambda package: package.version.without_local(),
Copy link
Member

Choose a reason for hiding this comment

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

That does not feel like a valid fix. I think it is not sufficient to group packages by name. Probably, they should be grouped by name and source. Then, the correct locked package can be chosen no matter what's the order in the lockfile.

@BeRT2me
Copy link
Author

BeRT2me commented Oct 6, 2023

Ye, I'm going to rethink this if I have time.

@BeRT2me BeRT2me closed this Oct 6, 2023
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.

3 participants