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

As of v2023.8.19 local file dependencies default to being installed as editable #6054

Closed
edmorley opened this issue Jan 4, 2024 · 10 comments
Labels
Category: Tests Relates to tests.

Comments

@edmorley
Copy link
Contributor

edmorley commented Jan 4, 2024

Issue description

Between Pipenv 2023.7.23 and 2023.8.19 there was an undocumented breaking change in default installation behaviour for local file directory installs that don't specify an explicit editable property, such as:

[packages]
mypackage = {file = "packages/mypackage"}

Before (in 2023.7.23 and earlier), such a dependency specifier was installed as a standard non-editable install.

Now (in 2023.8.19 and later), the install is performed as an editable install - ie: as though editable = "true" had been specified in the Pipfile / Pipfile.lock.

I presume this might be an unintended side-effect of #5793?

Expected result

If this behaviour change was intentional:

  • For it to have been documented in the changelog

If this wasn't intentional:

  • For the behaviour to have not changed
  • For Pipenv to have test coverage of this

And for bonus points either way:

Steps to replicate

Using https://github.com/edmorley/testcase-pipenv-file-editable:

Behaviour before (with 2023.7.23):

$ docker run --rm -it python:3.12 bash -c 'git clone -q https://github.com/edmorley/testcase-pipenv-file-editable && cd testcase-pipenv-file-editable && pip install -q --disable-pip-version-check --root-user-action=ignore pipenv==2023.7.23 && pipenv install && pipenv run pip show mypackage | grep -i location'
...
Location: /root/.local/share/virtualenvs/testcase-pipenv-file-editable-YkSiblPt/lib/python3.12/site-packages

Behaviour now (with 2023.8.19):

$ docker run --rm -it python:3.12 bash -c 'git clone -q https://github.com/edmorley/testcase-pipenv-file-editable && cd testcase-pipenv-file-editable && pip install -q --disable-pip-version-check --root-user-action=ignore pipenv==2023.8.19 && pipenv install && pipenv run pip show mypackage | grep -i location'
...
Location: /root/.local/share/virtualenvs/testcase-pipenv-file-editable-YkSiblPt/lib/python3.12/site-packages
Editable project location: /testcase-pipenv-file-editable/packages/mypackage

Note in the second output, the Editable project location shows that the package was installed in editable mode, when it was not before.

A package being installed in editable vs not editable mode has quite a few implications (such as whether the original location needs to be preserved for the environment to work, whether edits when developing take effect, plus the obvious path changes etc) - so a change like this is breaking, so should be mentioned in the changelog if it's intentional.

@edmorley edmorley changed the title As of 2023.8.19 local file dependencies default to being installed as editable As of v2023.8.19 local file dependencies default to being installed as editable Jan 4, 2024
@edmorley
Copy link
Contributor Author

edmorley commented Jan 4, 2024

In fact, it seems even if I explicitly specify editable = false (ie: mypackage = {file = "packages/mypackage", editable = false}), the dependency is still always installed in editable mode regardless?

edmorley added a commit to heroku/heroku-buildpack-python that referenced this issue Jan 5, 2024
Previously the buildpack would skip running `pipenv install` for repeat
Pipenv builds if (a) the SHA256 of the `Pipfile.lock` file had not changed
since the last successful build, and (b) there were no Git VCS references
in the lockfile.

However, this has a few issues:
1. There are other cases where it's not safe to assume that there is no
    need to re-run `pipenv install`, such as when installing a non-editable
    local dependency (see #1525), or when using editable local dependencies
    with the current path re-writing strategy (see #1520).
2. The current Git VCS check has false positives (see #1130, #1398).
3. Even if we try and add more checks/workarounds to resolve (1) and (2),
    we're still having to make assumptions about internal Pipenv implementation
    details, and hardcode these in the buildpack, hoping we didn't miss anything
    and that Pipenv's behaviour doesn't change over time (which is not the case,
    as seen by the recent regression pypa/pipenv#6054)

As such, we now instead always re-run `pipenv install`, and defer to Pipenv to
decide whether the environment needs updating.

This should still be fast, since the cached `site-packages` is still being used (and
if there are any scenarios in which it's not fast, then that's an upstream Pipenv
bug).

Integration tests were also added for various types of editable Pipenv installs,
since we previously only had test coverage of editable installs for Pip.

Fixes #1520.
Fixes #1525.
Closes #1130.
Closes #1398.
edmorley added a commit to heroku/heroku-buildpack-python that referenced this issue Jan 5, 2024
Previously the buildpack would skip running `pipenv install` for repeat
Pipenv builds if (a) the SHA256 of the `Pipfile.lock` file had not changed
since the last successful build, and (b) there were no Git VCS references
in the lockfile.

However, this has a few issues:
1. There are other cases where it's not safe to assume that there is no
    need to re-run `pipenv install`, such as when installing a non-editable
    local dependency (see #1525), or when using editable local dependencies
    with the current path re-writing strategy (see #1520).
2. The current Git VCS check has false positives (see #1130, #1398).
3. Even if we try and add more checks/workarounds to resolve (1) and (2),
    we're still having to make assumptions about internal Pipenv implementation
    details, and hardcode these in the buildpack, hoping we didn't miss anything
    and that Pipenv's behaviour doesn't change over time (which is not the case,
    as seen by the recent regression pypa/pipenv#6054)

As such, we now instead always re-run `pipenv install`, and defer to Pipenv to
decide whether the environment needs updating.

This should still be fast, since the cached `site-packages` is still being used (and
if there are any scenarios in which it's not fast, then that's an upstream Pipenv
bug).

Integration tests were also added for various types of editable Pipenv installs,
since we previously only had test coverage of editable installs for Pip.

Fixes #1520.
Fixes #1525.
Closes #1130.
Closes #1398.
@matteius
Copy link
Member

matteius commented Jan 27, 2024

Please help me to understand the use case(s) for wanting the local file dependency to not be "editable".

@matteius
Copy link
Member

I forget the exact reason I ran into why it ended up being to treat them all as editable, but I am willing to revisit -- I just feel I am lacking some context.

@edmorley
Copy link
Contributor Author

edmorley commented Jan 28, 2024

@matteius Thank you for replying.

It's slightly concerning to hear that this was in fact an intentional change after all, given that:

  1. It wasn't documented in the changelog or even talked about in the PR description (if this really was a design choice/tradeoff, than that's something that's best practice to mention in PR descriptions)
  2. Even without bugs, changing the default value for editable still would have been a breaking change
  3. It's currently buggy -- the editable option doesn't do anything at all but yet still exists. If the desire was not only to change the default, but also remove the ability to use non-editable with local file installs, than specifying editable: false should be an error and not silently ignored.

As for reasons why people use non-editable, I don't really have any insights into that. End users of ours reported breakages with their apps after a pipenv version upgrade. Pip supports installing any type of package (local, not local etc) in both editable and non-editable mode, as has earlier versions of pipenv. It's fine to remove a feature if it really does make implementation simpler (or if pipenv wants to be more opinionated than pip) - but this needs to be a carefully thought out, discussed and documented change.

@matteius
Copy link
Member

I understand what you are saying. The intent was never to remove the editable feature for local installs but I think the following things were in play:

  • Edge cases in requiermentslib (even before converting to pydantic) pre-existed with some local file install paths usages, was trying to fix.
  • Realized that fixing anything in requirementslib lead to cascading failures elsewhere because its a really confusing library--decided to work towards replacing it with functional methods within pipenv.
  • In parallel tried to address the deprecation warnings about new and old pip line syntax (removal of egg fragments) -- except that impacted things slightly, because not all pip lines even allow the new syntax.
  • Determining the package name of a local file install -- I think this was a big problem that still exists; I know pip does a better job determining the package name of a local file install, but also I think its partly because its not extracting that name out into lock metadata so early in the process -- would love to ride the rails of however pip is doing this though.

So then part of what happened is we got far enough converting off requirementslib that CIs were passing and the PR was huge that we shipped it, and there were a number of edge cases as it relates to vcs and local file installs and we shipped some hotfix type PRs for those edge cases. I was actually under the impression we had already fixed the editable vs not but that was likely only for VCS installs.

Trying to dig a bit deeper right now, its a bit more confusing as the logic for is_editable has been defined for a while (even before I moved it to its own utils file here) seemingly to imply all VCS and local installs are editable.
image

I don't have the exact answer right now, but I know it wasn't as simple as me thinking editable doesn't do anything, let's just remove it without saying anything; was more like I thought it was working right and not worse off than it was. Definitely I am revisiting though now.

@matteius
Copy link
Member

@edmorley I just opened a PR that modified that is_editable method that takes a pipfile entry to require editable be specified to be installed as editable. Sounds good, except there are other is_editable methods and code paths, and I don't have a great reproduction example or understanding of how to even tell if it gets installed one way or the other. Also I think there is the --ignore-pipfile option which probably impacts what it does.

@edmorley
Copy link
Contributor Author

edmorley commented Feb 1, 2024

I'm not entirely sure of the best way to check. It sounds like Pipenv has little to no existing test coverage of editable installs, which is a bit concerning? (Not meant as a criticism of yourself - I know how hard it is to try and sort out a codebase with lots of tech debt - particularly an open source project done in spare time. However, when end users have a choice between a handful of package managers, things like rate of regressions really can make a difference when deciding which to choose.)

The way editable installs are implemented are typically via .pth files in the site-packages directory - perhaps that's a quick way to tell?

If it were me, I'd probably check how pip or poetry write their tests for editable installs, and base them on that.

@matteius
Copy link
Member

matteius commented Feb 1, 2024

Just from glossing at it, it looks like there are tests asserting editable installs work, but not tests asserting anything about non-editable installs. Which I guess makes sense because the original issue is about these types of local and vcs installs getting treated as editable.
image

As for package managers -- I am really unopinionated what package manager a team chooses and for what reasons. On the one had I am Team pipenv: I want to see pipenv be the best it can be, which means adding tests and ensuring regressions get tested. However we also have almost 25K stars and relatively few contributions (though I think it has been trending up), they generally lack tests on average and we are willing to accept improvements in many cases without them, because as you mention, it sometimes means delaying an actual fix to OSS. On the other hand, I think what we have done with pipenv is put some life force back into it and create positive competition for other package managers to be better while trying to improve ourselves.

Just some of my thoughts: pipenv has an interesting history of being probably the first package manager outside of pip that tried to make it easier to manage larger groups of dependencies and provide hash checking and a lot of nice to haves with virtualenv and pyenv, that teams that were using requirements.txt without hashes at the time lacked the benefits of. It was oversold as being feature rich, when in fact it was buggy, but it also pioneered the way for other package managers to do things differently and in some cases even better from the get go. poetry now has twice the user base as pipenv, and I think that is great -- pipenv also has almost twice the user base in terms of downloads on pypi that it had two years ago. I think the best ecosystem is one where the projects can co-exist and benefit from each other in ways that you describe (like learning from what the other package managers are doing right). We often have looked at metrics like the python package manager shootout and pushed ourselves to do better. We have taken some stances on package confusion attacks and not leaving it up to random chance on those security concerns. Which has lead to some issue reports and has complicated workflows for some users of multiple package indexes, but that is a trade-off decision we made and hope to continue to push forward better solutions to make it easier to manage multiple indexes while still maintaining integrity of security.

All that being said, will it still annoy me when we get duplicate issue reports about the same complaints? Sure, for example, when some folks come here opening new issues about pipenv managing packages differently than their favorite javascript package manager, and claiming that these other package managers don't manage sub-dependencies of packages: I think they do on average, although perhaps differently): yeah, probably. But I am in it to make things better; one of the ways I can do that right now is by being as open and transparent that while I'll continue to try to improve test coverage, I won't be able to satisfy all constraints without more community contributions; even if those contributions are just to add missing test coverage, we will be grateful to have the help.

@matteius matteius added the Category: Tests Relates to tests. label Feb 7, 2024
@freddrake
Copy link

I have a use case for non-editable installs of packages from local files.

I wrote a tool to build Debian packages for applications using pipenv for dependency management (appackager, woefully underdocumented still). When writing an application, I've been using a locally-defined package that provides all the required entry points (esp. console_scripts) and a Pipfile that includes something like:

[packages]
mypackage = {path = ".", editable = true}

Then my Debian package builder re-writes the Pipfile.lock to set editable to false, builds a clean venv with pipenv sync, and uses that venv as the basis for the package.

With the change in pipenv 2023.8.19, additional work will have to be performed on the constructed venv to make it suitable. I've described the possible workaround in issue keepertech/appackager#5 of appackager.

@edmorley
Copy link
Contributor Author

I happened to spot #6222 in the changelog for Pipenv v2024.0.2, which seemed similar to this, so tested the repro here again.

It looks like #6222 fixed this, since the bug no longer reproduces in v2024.0.2 (but does in v2024.0.1):

$ docker run --rm -it python:3.12 bash -c 'git clone -q https://github.com/edmorley/testcase-pipenv-file-editable && cd testcase-pipenv-file-editable && pip install -q --disable-pip-version-check --root-user-action=ignore pipenv==2024.0.2 && pipenv install && pipenv run pip show mypackage | grep -i location'
...
Installing dependencies from Pipfile.lock (5c5ff4)...
Location: /root/.local/share/virtualenvs/testcase-pipenv-file-editable-YkSiblPt/lib/python3.12/site-packages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Tests Relates to tests.
Projects
None yet
Development

No branches or pull requests

3 participants