micropipenv icon indicating copy to clipboard operation
micropipenv copied to clipboard

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

Open wjhrdy opened this issue 2 years ago • 22 comments

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.

wjhrdy avatar Aug 12 '21 13:08 wjhrdy

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

goern avatar Aug 13 '21 07:08 goern

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.

fridex avatar Aug 16 '21 07:08 fridex

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?

frenzymadness avatar Aug 16 '21 07:08 frenzymadness

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.

fridex avatar Aug 16 '21 07:08 fridex

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.

frenzymadness avatar Aug 25 '21 12:08 frenzymadness

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).

fridex avatar Aug 25 '21 14:08 fridex

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

frenzymadness avatar Aug 25 '21 14:08 frenzymadness

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?

frenzymadness avatar Aug 30 '21 08:08 frenzymadness

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.

fridex avatar Aug 30 '21 08:08 fridex

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 avatar Sep 29 '21 11:09 sesheta

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

/close

sesheta avatar Sep 29 '21 11:09 sesheta

@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.

sesheta avatar Sep 29 '21 11:09 sesheta

/remove-lifecycle rotten /reopen /triage accepted

fridex avatar Sep 29 '21 12:09 fridex

@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 avatar Sep 29 '21 12:09 sesheta

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 :thinking:

frenzymadness avatar Oct 20 '21 11:10 frenzymadness

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.

fridex avatar Oct 21 '21 07:10 fridex

any update in this?

goern avatar Jan 14 '22 13:01 goern

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

frenzymadness avatar Jan 14 '22 15:01 frenzymadness

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 avatar Apr 14 '22 16:04 sesheta

/lifecycle frozen

harshad16 avatar Apr 18 '22 17:04 harshad16

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

goern avatar Jul 19 '22 15:07 goern

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

Let's reassign it. Thx!

fridex avatar Dec 07 '22 11:12 fridex