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

micropipenv._poetry2pipfile_lock() overrides python version specified by poetry.lock and pyproject.toml without warning #187

Open
wjhrdy opened this issue Aug 12, 2021 · 22 comments
Labels
human_intervention_required kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@wjhrdy
Copy link

wjhrdy commented Aug 12, 2021

Describe the bug
This line disregards the python version that might be specified in the poetry.lock -> [metadata] -> python-version without warning.

This line disregards the python version that might be specified in the pyproject.toml ->[tool.poetry.dependencies] -> python without warning.

It might be expected behavior to use the system python version over the specified version, but we might want a warning anyway.

@goern
Copy link
Member

goern commented Aug 13, 2021

/kind bug
/assign @fridex
/priority important-soon

@sesheta sesheta added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Aug 13, 2021
@fridex
Copy link
Collaborator

fridex commented Aug 16, 2021

Hi @wjhrdy, thanks for poetry-related reports. They sound reasonable. 👍🏻

It might be expected behavior to use the system python version over the specified version, but we might want a warning anyway.

We should fully reproduce the environment that poetry noted to create a reproducible environment. The fix should propagate the Python version from poetry files and not use the system Python version.

@frenzymadness
Copy link
Collaborator

Hi @wjhrdy, thanks for poetry-related reports. They sound reasonable. 👍🏻

It might be expected behavior to use the system python version over the specified version, but we might want a warning anyway.

We should fully reproduce the environment that poetry noted to create a reproducible environment. The fix should propagate the Python version from poetry files and not use the system Python version.

How do you plan to achieve that? I mean, micropipenv does not manage Python environments although it runs almost always in them. A warning saying something like "Dependencies definition has been prepared for Python X.Y but you are using X.Z" sounds reasonable. What else can micropipenv do?

@fridex
Copy link
Collaborator

fridex commented Aug 16, 2021

If I understood metadata stated in the pyproject.toml correctly, poetry notes down python version it used:

[tool.poetry.dependencies]
python = "^3.8"

We could propagate this information if stated - otherwise default to system python and print the warning as suggested. The version spec is probably another thing to deal with. I'm not familiar with poetry that much so more input is welcome.

@frenzymadness
Copy link
Collaborator

We could propagate this information if stated - otherwise default to system python and print the warning as suggested. The version spec is probably another thing to deal with. I'm not familiar with poetry that much so more input is welcome.

What do you mean by propagating it?

Right now, micropipevn uses pip command which usually means that it installs packages for the main Python interpreter for which the main pip command is installed or it installs packages inside a virtual environment that provides its own pip command for whatever Python version is has been created for.

I'm afraid there is nothing we can do about it. We cannot search for another alternative Python interpreter when micropipenv runs in a virtual environment. In that case, printing a warning is all we can do and it might make sense.

When running outside a virtual environment, we can actually try to find the correct Python version specified in the configuration and then use pythonX.Y -m pip instead of pip itself but I'm not sure whether this is a good idea. On Fedora, you can have multiple Pythons installed and it'd be very surprising for me if a command like micropipenv install --user would install packages for example for Python 3.5 just because I forgot to change the version in an upstream config file.

@fridex
Copy link
Collaborator

fridex commented Aug 25, 2021

OK, I think we have a communication noise here - by propagating, I meant reading Python version from poetry files and adding it to Pipfile.lock format in micropipenv._poetry2pipfile_lock. Then other parts of micropipenv will pick this Python version information and print the warning or fail in case of --deploy flag (similarly as we error out in case of Python version mismatch in Pipfile.lock).

@frenzymadness
Copy link
Collaborator

Thanks for the clarification. Now it makes perfect sense and I'll take a look at it.

@frenzymadness
Copy link
Collaborator

I looking at it. If python in [tool.poetry.dependencies] in pyproject.toml contains something like "3.6", the same value is in poetry.lock in [metadata] section under the python_versions key and we can easily take it and put it into Pipfile.lock under _meta - requires - python_version.
The problem arises when a version in pyproject.toml and poetry.lock is something like ^3.7. We do not support caret specifiers yet (#33 ) and I cannot find a specification of Pipfile.lock to see what schemas are supported there. Moreover, caret specifiers should work like this:

  • ^1.2.3 is the equivalent for >=1.2.3, <2.0.0
  • ^0.2.3 is the equivalent for >=0.2.3, <0.3.0
  • etc.

but I believe that ^3.7 should mean >=3.7, <3.8 instead of >=3.7, <4.0 so the handling of the caret specifier should behave differently for Python and other versions. Do I understand it correctly? Is something like >=3.7, <4.0 supported in Pipfile.lock?

@fridex
Copy link
Collaborator

fridex commented Aug 30, 2021

Is something like >=3.7, <4.0 supported in Pipfile.lock?

I don't think so - Pipenv does not support version range specification on Python interpreter version. We could stick with the lowest Python interpreter version that is used in poetry as that is used as a base for python version range specification if I understand poetry behaviour correctly - e.g. if someone starts a project on Python 3.7, poetry injects ^3.7 by default thus micropipenv could use 3.7 in Pipfile.lock with a warning about the conversion done.

I'm unsure if poetry supports something like ^3.7,<=3.8.2 or >=3.6,<=3.8 in the Python version range specifier though.

@sesheta
Copy link
Member

sesheta commented Sep 29, 2021

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle rotten

@sesheta
Copy link
Member

sesheta commented Sep 29, 2021

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

@sesheta sesheta added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Sep 29, 2021
@sesheta sesheta closed this as completed Sep 29, 2021
@sesheta
Copy link
Member

sesheta commented Sep 29, 2021

@sesheta: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@fridex
Copy link
Collaborator

fridex commented Sep 29, 2021

/remove-lifecycle rotten
/reopen
/triage accepted

@sesheta
Copy link
Member

sesheta commented Sep 29, 2021

@fridex: Reopened this issue.

In response to this:

/remove-lifecycle rotten
/reopen
/triage accepted

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sesheta sesheta reopened this Sep 29, 2021
@sesheta sesheta added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Sep 29, 2021
@frenzymadness
Copy link
Collaborator

I've tested that Python versions like ">=3.7,<=3.10.2" or "^3.7,<=3.10.2" or even "~2.7 || ^3.4" are supported by poetry and poetry just moves that information from pyproject.toml to poetry.lock. Specification for Pipfile says that the Python version has to be something like "X.Y" and/or python_full_version something like "X.Y.Z" so we have to find a way how to distill one exact Python version from the possible specifications allowed in poetry.

I don't want to reimplement the wheel so I'm thinking about regex like: r"(\S{0,2})(3\.\d+)" which would find the first Python 3 version in the specification and possibly some chars modifying the version range. If the modifier equals to !=, I'd skip it and find another version. We can implement also handlers for < or > but I believe that the first version in the range should always have at least >= or <= for the first specified version so there should never be something like <3.9 or >3.10.

In the other hand, if we want to implement #189 we need better handling of the version ranges. But the code to handle this in packaging or poetry.core is very complex and definitely something we do not want to bring here or depend on.

We can implement something simple and show a warning if we are not able to parse the version specifier but what is simple and reasonable at the same time?

What else we can do? We can just show a warning to users saying that they are responsible for checking the Python version in poetry.lock because we are not able to parse it here and ignore that information as we currently do 🤔

@fridex
Copy link
Collaborator

fridex commented Oct 21, 2021

In the other hand, if we want to implement #189 we need better handling of the version ranges. But the code to handle this in packaging or poetry.core is very complex and definitely something we do not want to bring here or depend on.

We can implement something simple and show a warning if we are not able to parse the version specifier but what is simple and reasonable at the same time?

+1 for a simple solution with a warning, especially at the beginning.

I'm wondering if we could reuse logic from packaging. Recent pip versions vendor it, but I'm unsure if all the versions of pip that micropipenv is compatible with would provide desired functionality. With some effort, we could eventually translate caret, tilde, and wildcard requirements Poetry has to Python's packaging compatible representation. However, this will definitely require some effort to create such a parser (maybe some parts could be reused from Poetry). A fully compatible solution might turn to be complex.

@goern
Copy link
Member

goern commented Jan 14, 2022

any update in this?

@frenzymadness
Copy link
Collaborator

Not really. I'm still not sure how to implement this.

@sesheta
Copy link
Member

sesheta commented Apr 14, 2022

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@sesheta sesheta added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 14, 2022
@harshad16
Copy link
Member

/lifecycle frozen

@sesheta sesheta added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 18, 2022
@goern
Copy link
Member

goern commented Jul 19, 2022

@fridex could you have a look at this, or is it better to reassign it? thx!

@fridex
Copy link
Collaborator

fridex commented Dec 7, 2022

@fridex could you have a look at this, or is it better to reassign it? thx!

Let's reassign it. Thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
human_intervention_required kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

6 participants