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

config: introduce installer.no-binary #5609

Merged
merged 1 commit into from
May 14, 2022

Conversation

abn
Copy link
Member

@abn abn commented May 14, 2022

This change replaces the --no-binary option introduced in #5600 as the original implementation could cause inconsistent results when the add, update or lock commands were used.

This implementation makes use of a new configuration installer.no-binary to allow for user specification of sdist preference for select packages.

References: #5600 #5599 (comment)
Relates-to: #1316 #5599 #365

@abn abn requested a review from a team May 14, 2022 16:23
@abn abn added the area/docs Documentation issues/improvements label May 14, 2022
@github-actions
Copy link

github-actions bot commented May 14, 2022

Deploy preview for website ready!

✅ Preview
https://website-j1y1xefak-python-poetry.vercel.app

Built with commit 2a2e4fe.
This pull request is being automatically deployed with vercel-action

@abn
Copy link
Member Author

abn commented May 14, 2022

This change replaces the `--no-binary` option introduced in python-poetry#5600 as
the original implementation could cause inconsistent results when the
add, update or lock commands were used.

This implementation makes use of a new configuration
`installer.no-binary` to allow for user specification of sdist
preference for select packages.
@abn abn force-pushed the install-no-binary branch from b38709e to 2a2e4fe Compare May 14, 2022 16:32
@abn abn added area/config Related to configuration management area/installer Related to the dependency installer labels May 14, 2022
@pmav99
Copy link
Contributor

pmav99 commented May 14, 2022

@abn I tested it. As far as I can tell it works great and the docs are clear. Kudos!

As I mentioned in the issue, my only concern is whetherpoetry.toml is the proper place to introduce an option about the installation of specific packages. A possible alternative could be to have an extra section in pyproject.toml. E.g.

[tool.poetry.no-binary]
packages = [
    "shapely",
    "pygeos",
]

From a UX point of view, both approaches should be practically equivalent. The poetry.toml one has the advantage that the option can be set via the CLI, too. The pyproject.toml avoids the proliferation of configuration options in poetry config.

That being said, you have a much better understanding of the "spirit" of poetry's configuration so I trust your judgement.

@abn
Copy link
Member Author

abn commented May 14, 2022

Right now I think this is the best route as the alternative might set incorrect expectations and cause unintended side-effects. Nothing stopping us from iterating on this. But watch this space :).

@neersighted neersighted merged commit ed26721 into python-poetry:master May 14, 2022
@abn abn deleted the install-no-binary branch May 15, 2022 00:03
@sneakers-the-rat
Copy link
Contributor

out of curiosity, can I ask why not have it as an option on a specific dependency? so you could do something like specifying no-binary for a particular platform, etc.?

[tool.poetry.dependencies]
numpy = [
  { version = "^1.20.0", no_binary = true, markers="sys_platform == 'win32'" },
  { version = "^1.20.0", markers="sys_platform != 'win32'" }
]

@abn
Copy link
Member Author

abn commented May 17, 2022

Part of the reasons are mentioned in #5599 (comment). I do not think this is completely infeasible, however not something we would want to implement right now. And I am still not convinced this is upto the project to decide as this.

In addition to the above, the solution in this PR also applies to indirect dependencies as well. Because, often times the issues faced by users are to do with a package that are not a dependency of the package itself. Ofcourse, you can add it explicitly to the package. So, the inconsistencies it can introduce and lack of an implementation that addresses all concerns are reasons why the dependency specific configuration is not prefered at this point in time.

@rjurney
Copy link

rjurney commented Aug 19, 2022

@abn this isn't working in 1.2b3 for me...

Ohhhhh poetry.toml !

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/config Related to configuration management area/docs Documentation issues/improvements area/installer Related to the dependency installer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants