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

poetry should not prepend sys.path with __vendor_site__ #8157

Open
mhils opened this issue Jul 5, 2023 · 7 comments · May be fixed by python-poetry/poetry-core#753
Open

poetry should not prepend sys.path with __vendor_site__ #8157

mhils opened this issue Jul 5, 2023 · 7 comments · May be fixed by python-poetry/poetry-core#753
Assignees
Labels
area/core Related to the poetry-core library area/deps Related to representing and locking dependencies kind/bug Something isn't working as expected status/triage This issue needs to be triaged

Comments

@mhils
Copy link

mhils commented Jul 5, 2023

Problem Description

poetry-core currently prepends its _vendor directory (__vendor_site__) to sys.path here. This causes poetry plugins to pick up whatever dependency version poetry has vendored, which may not be compatible with what the plugin requires / states in its dependencies.

For example, here's what we get after trying to update our poetry plugin to pydantic 2.0, which imports TypeAliasType from typing_extensions. Poetry's 1.6.1 vendored copy does not include TypeAliasType, so every poetry command crashes immediately:

$ poetry help

cannot import name 'TypeAliasType' from 'typing_extensions' (/home/user/venv/lib/python3.11/site-packages/poetry/core/_vendor/typing_extensions.py)

This could be fixed by vendoring similar to how pip does it, i.e. without modifying sys.path but by explictly importing from poetry.core._vendor.

@mhils mhils added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Jul 5, 2023
@dimbleby
Copy link
Contributor

dimbleby commented Jul 5, 2023

That would not be enough: eg the reason for vendoring typing-extensions is not that poetry-core depends on it directly, but that the vendored jsonschema depends on it.

Arguably poetry-core ought to patch jsonschema to pick up the vendored typing-extensions, but this is certainly going to get tedious...

fwiw the main branch of poetry-core doesn't vendor typing-extensions any more, so this particular issue will go away.

@mhils
Copy link
Author

mhils commented Jul 6, 2023

I agree this is not particularly fun to fix, but mangling sys.path the way it is done right now is practically guaranteed to break other poetry plugins in the future. :/

@dimbleby
Copy link
Contributor

dimbleby commented Jul 6, 2023

I suspect plugin authors will find it more efficient in the rare cases where this happens to submit an MR that simply updates the packages that poetry-core vendors. Or: to sit and wait for someone else to do that!

I guess you will be the testcase for this theory...

@mhils
Copy link
Author

mhils commented Jul 6, 2023

typing-extensions was already removed from _vendor when I started to look into this, so there was no opportunity for me to submit a PR that simply updates the package. As a plugin author, my feedback is that I would find it even more efficient if I would get the dependencies I actually declared on import.

My (hopefully constructive) feedback is that pip has a different approach1, which seems to work reasonably well. I appreciate y'all's work on poetry, I just don't particularly appreciate poetry fiddling with my sys.path. :-)

Footnotes

  1. https://github.com/pypa/pip/blob/main/src/pip/_vendor/README.rst

@dimbleby
Copy link
Contributor

dimbleby commented Jul 6, 2023

Not arguing with your feedback, rather trying to be realistic about what's likely to result from it. We agree that the solution you are suggesting is likely to be tedious / not fun to fix: therefore the only way it happens is if someone who is motivated to make it happen shows up.

At the moment you are the leading candidate, and you seem to be counting yourself out.

Agree that this is a legit report but - like most of the backlog - I wouldn't expect that simply reporting it is likely to lead to action.

@mhils
Copy link
Author

mhils commented Jul 6, 2023

I wouldn't expect that simply reporting it is likely to lead to action.

Perfectly fine, no expectations. I'm doing enough FOSS myself. :)

We agree that the solution you are suggesting is likely to be tedious / not fun to fix:

More importantly, tedious to maintain. Doing a one-pass to fix it is releatively easy. But I can't commit to any long-term help, and it arguably makes all vendored dependency updates more time-consuming going forward. And I don't want to put that burden onto anyone without an explicit "yes, that's a tradeoff we want" from the maintainers.

@Secrus
Copy link
Member

Secrus commented Aug 30, 2024

It took a good moment but I finally got to changing it. python-poetry/poetry-core#753

@Secrus Secrus added area/core Related to the poetry-core library area/deps Related to representing and locking dependencies labels Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core Related to the poetry-core library area/deps Related to representing and locking dependencies kind/bug Something isn't working as expected status/triage This issue needs to be triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants