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

Isolate conda python from site packages, like venv does by default #24506

Closed
wants to merge 10 commits into from

Conversation

itcarroll
Copy link

@itcarroll itcarroll commented Nov 15, 2023

The package (which is a one liner) addresses an issue repeatedly brought up in conda/conda#394, conda/conda#448, conda/conda#7707, conda/conda#7173, and conda/conda#13337. The solution is different from the solution introduced in the conda-ecosystem-user-package-isolation package; it only effects Python in the environment. All this package does it does during buildconda create, which is to create/append the line "include-system-site-packages = false" to a "pyvenv.cfg" file read by the site module from the Python standard library. create a "pyvenv.cfg" file, if one does not exist, with the line "include-system-site-packages = false" via a conda plugin implementing a post-command extension to conda create. As a result this package, if installed in the base environment,

  • affects all new environments created by conda that include Python
  • does not affect Python in the base environment
  • will cease to have any effect if there is a change to conda that adds a pyvenv.cfg file during python installation.

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source. Not Applicable
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details). Not Applicable
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/conda-ecosystem-site-packages-isolation) and found some lint.

Here's what I've got...

For recipes/conda-ecosystem-site-packages-isolation:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

Copy link
Contributor

Hi! Thanks for your contribution to conda-forge.
When submitting a pull request, please do not change anything in the example recipe.
Please make sure that any changes are reverted before you submit it for review.
Thanks!

@itcarroll
Copy link
Author

I am willing to be a listed maintainer.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/conda-ecosystem-site-packages-isolation) and found it was in an excellent condition.

@itcarroll

This comment was marked as outdated.

@chrisburr
Copy link
Member

Is this editing a file in-place? If so, you can't rely on the installation order so people could get either the fixed or unfixed version.

@jaimergp
Copy link
Member

AFAIK this is shipping the file, which is subject to clobbering. Having no deps, maybe it's installed last or first, not sure how the toposort treats lone leaves.

An alternative is to use post-link.sh to edit things at env creation time, but it might suffer from the same clobbering problems.

Realistically speaking I don't know if there are many packages clobbering pyenv.cfg, so it's not that bad, but I'm not sure about how good a solution this is, given this would be opt-in for every environment created and tied to the conda-forge channel when it's more of a ecosystem-wide configuration aspect.

A conda specific alternative would be to have:

  1. A post-solve plugin hook
  2. A plugin implementing such hook, watching LINK transactions. If python is included, it ensures pyvenv.cfg is configured properly via python -mvenv.

Another option is to have this depend on python and use python -mvenv to do all the configuration. But again, nothing guarantees those changes are undone by other packages.

At any rate, if this goes through, we need to be careful about where in the prefix tree this is placed, because according to PEP 405, it might change the values of sys.prefix and friends.

@itcarroll
Copy link
Author

If there is interest here in a conda solution, please comment on conda/conda#13337.

@itcarroll
Copy link
Author

I think I see two concerns about clobbering a pyvenv.cfg file.

  1. If this package is installed before Python, it might have no effect.
  2. If another package is installed after this one, it might undo this package's effect.

I don't think 1 is a problem, because the $PREFIX directory exists for any conda environment and I don't think installation of Python would remove an existing pyvenv.cfg file.

I guess for 2 I'm not sure that we want to prevent users from altering a configuration file. The point of this package is to make it easy to create. If a user decides later they want a different configuration, that's okay.

@mfansler
Copy link
Member

I agree @jaimergp that it makes more sense to implement the changes in a hook rather than ship as a static file. If one really wanted to enforce the status, one could check it (and potentially correct it) via an activation script.

@itcarroll
Copy link
Author

itcarroll commented Nov 17, 2023

I'm not sure I understand what the suggestions to use a hook imply regarding a conda-forge package, but I believe the two alternatives proposed above (the post-solve plugin hook, or the ... well, other one) are ideas for a solution on conda itself. I fully support a fix over there, and encourage closing this PR if anyone can work up a conda solution. It's beyond me.

@mfansler
Copy link
Member

@itcarroll apologies - I was perhaps a bit loose in referring to package scripts as "hooks". I'll clarify, since indeed I was still talking about this package.

Currently, this recipe ships a static pyvenv.cfgfile. So if other recipes do something similar, the ultimate installed version in an environment will depend on (possibly arbitrary) installation order.

Jaime suggested to instead use a post-link.sh script (after the package has been technically "installed") to generate/edit a pyvenv.cfg on-the-fly. That avoids this package overwriting others, but does nothing to prevent our setting from being wiped. (Jaime also had suggestions for an alternate approach via a plugin instead of a package.)

I then suggested this package could also use an activate.sh script to keep the setting enforced. That is, the setting value could be checked and repaired, rather than setting it only once and hoping nothing messes it up. To me this seems on par with setting environment variables during environment activation, but I'd like to know if others see it differently.

Anyway, I think of these scripts as using "hooks" (invoked on events), but realize the Conda documentation has a precise meaning of "hook" in the context of plugin API. Mea culpa!

@itcarroll itcarroll marked this pull request as draft November 18, 2023 19:56
@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/conda-create-no-user-site) and found some lint.

Here's what I've got...

For recipes/conda-create-no-user-site:

  • requirements: run_constrained: python>=3.3 must contain a space between the name and the pin, i.e. python >=3.3
  • If python is a host requirement, it should be a run requirement.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/conda-create-no-user-site) and found it was in an excellent condition.

@itcarroll
Copy link
Author

itcarroll commented Nov 21, 2023

Thanks all for the suggestions and help. I have refactored the package as a post-command plugin to conda create, but continue to "ship" the pyvenv.cfg file. I don't see any alternative to using a pyvenv.cfg to configure the search paths; Python's mechanism is based on this file. By changing the package to a post-command hook that only acts during environment creation, I believe it is now also clear that the package will not try to prevent clobbering of the pyvenv.cfg. This behavior is how python -m venv works too, there are no guard rails on the configuration file.

I considered the advantages and disadvantages of using venv to create the configuration file, and opted to create the configuration file manually. The venv module is inflexible about including lines that do not apply to conda environment creation, so the manual approach is cleaner than trying to prune a file created by venv.

@itcarroll itcarroll marked this pull request as ready for review November 21, 2023 14:11
@itcarroll
Copy link
Author

@conda-forge/help-python, ready for review!

- python -c "import sys, site; sys.exit(0 if site.ENABLE_USER_SITE==False else 1)"

about:
home: http://conda-forge.org/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
home: http://conda-forge.org/
home: http://github.com/conda-forge/conda-create-no-user-site-feedstock

summary: conda create will configure Python to ignore packages outside the environment
description: >
This package extends `conda create` to configure Python so it will ignore packages
installed with `pip install --user`. Envrionment creation will now add a
Copy link
Member

@jaimergp jaimergp Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
installed with `pip install --user`. Envrionment creation will now add a
installed with `pip install --user`. Environment creation will now add a

This package extends `conda create` to configure Python so it will ignore packages
installed with `pip install --user`. Envrionment creation will now add a
prefix-level "pyvenv.cfg" file with the line `include-system-site-packages = false`,
which gets read by Python's `site` module whenver Python is run. This aligns Python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
which gets read by Python's `site` module whenver Python is run. This aligns Python
which gets read by Python's `site` module whenever Python is run. This aligns Python

yield plugins.CondaPostCommand(
name="no-user-site",
action=action,
run_for={"create"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also consider adding install, remove and maybe even update. The reasons:

  • A user had created an environment without Python, but then installs something that brings Python in.
  • The user removes python from the environment, and there's a lingering pyvenv.cfg file, which should be removed if possible.

version: 0.1.0

source:
path: .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the maintenance model for this package? Feedstocks are not usually equipped to deal with release processes and PRs, so maybe it's a better idea to commit this to a separate repo (maybe under conda-incubator, if you are inclined). I like seeing more conda plugins in the world and it would help with visibility.

@@ -0,0 +1,44 @@
package:
name: conda-create-no-user-site
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel a better name would be conda-pyvenv-compat if that's the focus? Or conda-no-user-site?

@jezdez
Copy link
Member

jezdez commented Feb 15, 2024

Hi there, conda maintainer here (and former virtualenv developer):

I believe this proposed package is solving the problem on the wrong level as it's using the plugin system to change a fundamental feature of conda and in effect makes you the maintainer of this pattern in the conda ecosystem going forward. That's not sustainable and would be better solved by a config option in conda instead. It would also make this available for user of conda that aren't using conda-forge, e.g. users of Anaconda Distribution.

I would encourage you instead to work on a patch to conda to fix conda/conda#13337 via a condarc setting that would enable users to switch to include-system-site-packages=false if they choose to do so. We can document it in the official conda docs and point out pros and cons there, when creating that feature as well, increasing the chance that users will know about it.

Maybe, when we get feedback from users, we can even change the default for new conda installations in the future. Happy to discuss what we would need to know to make that decision.

Hope that helps!

@itcarroll
Copy link
Author

Thank you @jaimergp and @jezdez for the review and comment! I would also much prefer to see a solution at the conda level, or perhaps on the anaconda/python "feedstock" (if there is such a thing for anaconda packages outside conda-forge) since it is specific to environments with Python. I started this package because I got zero support on conda/conda#13337. If you can generate some interest on the conda issue, we can continue finding the right solution over there and I'll just close this PR.

@itcarroll
Copy link
Author

Closing as won't fix in deference to conda/conda#13635. Thanks for the support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants