Implement config file location 'base' to config all environments of an interpreter #11487

pelson · 2022-10-05T16:23:57Z

Follows-on from the discussion in #9752.

@pradyunsg has expressed a willingness to review this change.

pradyunsg

Thanks for filing this!

I'd like to request you to improve the commit message.

news/9752.feature.rst

pradyunsg · 2022-10-05T19:11:25Z

src/pip/_internal/configuration.py

@@ -344,6 +347,8 @@ def iter_config_files(self) -> Iterable[Tuple[Kind, List[str]]]:
            # The legacy config file is overridden by the new config file
            yield kinds.USER, config_files[kinds.USER]

+        yield kinds.BASE, config_files[kinds.BASE]


The "base" location should be skipped outside of virtual environments, where base_prefix and prefix are the same.

We should also provide a (temporary) opt-out from this, for users who might find this change to be disruptive.

The "base" location should be skipped outside of virtual environments, where base_prefix and prefix are the same.

I considered this before opening the PR, but the problem is that we don't know what config files need to be loaded at this point - there may be a load-only of BASE, which would mean we don't get that config file loaded in non-venv contexts. Perhaps that is OK though - I would add a caveat to the docs to say BASE is only considered when a virtual environment is active.

I was further dissuaded from my implementation by the todo at the top of the function, which asks for less logic in what config files to choose https://github.com/pypa/pip/pull/11487/files#diff-eadf79f17f8c7b56d3743fb4bb0ef3c2c2e062e493e8c25962f3319ef264f8bfR335.

We should also provide a (temporary) opt-out from this, for users who might find this change to be disruptive.

I agree. I could add something specific to BASE, or I could look into allowing the precedence order to be overridden (and to disallow certain levels). Something like:

config-precedence = env global user site env_var

That would amount to quite a big change though - and might require some thought about doing the config loading in two passes (first to determine the precedence, then to actually load it in the desired order) - this would reduce to a single pass in the case that the precedence isn't overridden. I would be happy to talk about this offline/elsewhere if this is something you would like me to pursue.

If you want me to keep it BASE scoped (and simpler), then suggestions are very welcome on the best approach - a simple env var, for example?

I took a poke at implementing config-precedence config at pelson@0553eb1. I think it turned out quite well in fact - might be a nice alternative option for allowing users to opt-out of BASE loading.

I would be happy to discuss this on a separate issue, or on discord, if this is interesting to pursue.

Since the temporary opt-out comment is withdrawn in #11487 (comment), the remainging discussion is:

The "base" location should be skipped outside of virtual environments, where base_prefix and prefix are the same.

In my response, I highlight that this particular function has a comment:

# SMELL: Move the conditions out of this function

I agree with the suggestion that the function smells, as the function's objective should be to unconditionally enumerate the locations that configuration may come from, with _load_config_files determining if those files should be loaded or not.

For this reason, I added the BASE config file unconditionally. If we want to optimise and avoid BASE and SITE being loaded if they are the same, then I would put this logic in the _load_config_files method. Would be happy to add this, if it is considered important.

pradyunsg · 2022-10-05T19:12:43Z

docs/html/topics/configuration.md

-: {file}`%VIRTUAL_ENV%\\pip.ini`
+: {file}`\{sys.prefix\}\\pip.ini`


Don't change this, since VIRTUAL_ENV is more readily understood to be the directory containing the virtualenv.

I agree on the understanding, but until I re-read https://docs.python.org/3/library/venv.html#creating-virtual-environments, thought you were technically it is wrong.

It turns out that it is indeed documented that %VIRTUAL_ENV% must be set (this has been documented since Python 3.8). Unfortunately that is a bug in the Python docs, AFAICS - there is no actual implementation to ensure that VIRTUAL_ENV is actually set:

$ python3.9 -m venv /tmp/a-new-venv $ /tmp/a-new-venv/bin/python -c "import os; print(f'Val: {os.getenv(\"VIRTUAL_ENV\")}')" Val: None

Therefore the CPython documentation that states:

When a virtual environment is active, the VIRTUAL_ENV environment variable is set to the path of the virtual environment. This can be used to check if one is running inside a virtual environment.

Is sadly misguided - it isn't safe to use that environment variable to determine virtual-environment-ness IMO (and using env-vars to determine that sounds like a bad idea anyway - residual environment variables are a concern there).

(btw: Recommended it gets reverted in python/cpython#21970 (comment))

OK that is a bit of an aside. I'm open to reverting to %VIRTUAL_ENV%, but want to recognise that it is not strictly accurate, and that to document the base location, I will be using sys.base_prefix anyway.

Do you still want me to revert?

As this is documentation, and $VIRTUAL_ENV is commonly understood (right or not) to mean the virtual environment's directory, I agree with @pradyunsg that this would be better reverted (in the 3 places it occurs)

Thanks for the clarity. Commit added to this tune.
FWIW, in the meantime, I also clarified the Python documentation on the use of the VIRTUAL_ENV environment variable at python/cpython#98350.

pradyunsg · 2022-10-05T19:13:04Z

docs/html/topics/configuration.md

-: {file}`$VIRTUAL_ENV/pip.conf`
+: {file}`\{sys.prefix\}/pip.conf`


Don't change this, since VIRTUAL_ENV is more readily understood to be the directory containing the virtualenv.

docs/html/topics/configuration.md

tests/unit/test_configuration.py

…rtual environments sharing the same base. The new functionality serves a use case which was not previously possible with pip configuration files, namely the situation where you have a base Python installation and want to influence the pip configuration for all derivative virtual environments *without* changing the config for all other environments on a machine (global), or for all other environment run by the same user (user). Concretely, this could be used for a centrally managed network mounted filesystem based Python installation, from which multiple users can build virtual environments and inside which a specific pip configuration is needed (e.g. an index URL).

pelson · 2022-10-06T12:28:58Z

Note: Might want to add this as an option in

pip/src/pip/_internal/commands/configuration.py

Line 146 in 7311c82

(kinds.GLOBAL, options.global_file),

docs/html/topics/configuration.md

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

uranusjr · 2022-11-16T20:56:52Z

This is good enough for me but still needs to get a pass from others.

pfmoore · 2022-11-16T21:02:27Z

I can no longer find my previous review comments. Presumably some force-push replaced the code that I was concerned about, but is there a way I can review what I said previously, to confirm I'm happy ny concerns are resolved?

uranusjr · 2022-11-17T01:56:55Z

Hmm, force-pushes shouldn’t affect reviews, so if there’s comment missing it’s likely a GitHub issue and can’t be retrieved.

pelson · 2022-11-17T12:54:32Z

The only comment @pfmoore was #11487 (comment) (in relation to the version that this will be available). I get notifications through email, so have an archive. Perhaps you didn't hit submit on previous comments? Or perhaps you are thinking of the comments in the issue #9752?

@pradyunsg had a few comments which are outstanding. A brief summary:

Use %VIRTUAL_ENV% instead of {sys.prefix} (I commented that this technically not 100% incorrect, but probably clearer to a beginner reader, so a decision needed)
Allow opt-out of the BASE config loading. I have a general proposal which allows overriding the config loading order. I have a demonstration implementation (link above) for the principle, but in practice it needs a bit more work to implement the whole thing properly, including tests (in my implementation, I didn't fully work through the priority resolution of specific command config).
- In practice, it will be much simpler to have a simple feature flag, but I would appreciate advice in order to take the same approach as other feature flags in pip

pfmoore

Apart from one comment which @pradyunsg already covered, this LGTM

pfmoore · 2022-11-17T14:35:53Z

docs/html/topics/configuration.md

-: {file}`%VIRTUAL_ENV%\\pip.ini`
+: {file}`\{sys.prefix\}\\pip.ini`


As this is documentation, and $VIRTUAL_ENV is commonly understood (right or not) to mean the virtual environment's directory, I agree with @pradyunsg that this would be better reverted (in the 3 places it occurs)

pfmoore · 2022-11-17T14:39:02Z

Perhaps you didn't hit submit on previous comments?

No, that'll be it - github says I have unresolved comments and wouldn't tell me what they were, was all.

…tation This follows the discussion in https://github.com/pypa/pip/pull/11487/files#r988625394, that despite the VIRTUAL_ENV environment variable not being the technically correct value, it is more readily understood by readers than ``sys.prefix``.

pradyunsg · 2023-01-25T19:10:17Z

Looking at this now... it's probably fine to not have any opt-out on this.

I'm definitely a strong -1 to any mechanism allowing users to control the precedence order -- that sounds like a recipe for introducing wayyyy more complexity for sysadmins who wish to have some amount of control over how pip is invoked on a shared machine (eg: in a university or whatnot).

pradyunsg · 2023-01-25T19:11:24Z

FWIW, I'm happy to let someone else land this. I'll have limited bandwidth over the coming daays and will likely focus on making a release with that bandwidth. :)

pelson · 2023-01-26T10:22:36Z

@pradyunsg - thanks for your input. Totally understand about your limited bandwidth.

Looking at this now... it's probably fine to not have any opt-out on this.

Then I don't think there are any outstanding actions on this PR (I have clarified one point regarding optimising the load of BASE and SITE if they are the same thing, which needs to be done carefully, and I'm not sure is actually worth doing).

If there is any more I can do to get this over the line, then please let me know - it will be great to solve this multi-year issue 👍

uranusjr · 2023-01-31T17:40:01Z

With 23.0 out I’ll just merge this. We have another three months before this is actually released, and it’s easier to test things with this incorporated instead of in a separate branch.

pradyunsg reviewed Oct 5, 2022

View reviewed changes

uranusjr reviewed Oct 6, 2022

View reviewed changes

docs/html/topics/configuration.md Outdated Show resolved Hide resolved

pelson force-pushed the feature/base-prefix-config branch from caddbb1 to ad9b42e Compare October 6, 2022 06:56

pelson commented Oct 6, 2022

View reviewed changes

tests/unit/test_configuration.py Show resolved Hide resolved

pelson force-pushed the feature/base-prefix-config branch from ad9b42e to b777bcd Compare October 6, 2022 07:37

Merge branch 'main' into feature/base-prefix-config

4a87ab0

uranusjr reviewed Nov 16, 2022

View reviewed changes

docs/html/topics/configuration.md Outdated Show resolved Hide resolved

Update docs/html/topics/configuration.md

93ade85

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

uranusjr approved these changes Nov 16, 2022

View reviewed changes

pfmoore approved these changes Nov 17, 2022

View reviewed changes

pelson and others added 2 commits November 17, 2022 17:31

Merge branch 'main' into feature/base-prefix-config

8d34a11

uranusjr changed the title ~~Implement the new base prefix config "level", allowing a single config file for all venvs sharing the same base~~ Implement config file location 'base' to config all environments of an interpreter Jan 31, 2023

uranusjr merged commit 56e5fa3 into pypa:main Jan 31, 2023

pelson deleted the feature/base-prefix-config branch January 31, 2023 19:48

pelson mentioned this pull request Jan 31, 2023

Providing pip configuration in sys.base_prefix #9752

Open

1 task

github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement config file location 'base' to config all environments of an interpreter #11487

Implement config file location 'base' to config all environments of an interpreter #11487

pelson commented Oct 5, 2022

pradyunsg left a comment

pradyunsg Oct 5, 2022

pradyunsg Oct 5, 2022

pelson Oct 6, 2022

pelson Oct 6, 2022

pelson Jan 26, 2023

pradyunsg Oct 5, 2022

pelson Oct 6, 2022 •

edited

Loading

pfmoore Nov 17, 2022

pelson Nov 17, 2022

pelson Jan 26, 2023

pradyunsg Oct 5, 2022

pelson Jan 26, 2023

pelson commented Oct 6, 2022

uranusjr commented Nov 16, 2022 •

edited

Loading

pfmoore commented Nov 16, 2022

uranusjr commented Nov 17, 2022

pelson commented Nov 17, 2022

pfmoore left a comment

pfmoore Nov 17, 2022

pfmoore commented Nov 17, 2022

pradyunsg commented Jan 25, 2023

pradyunsg commented Jan 25, 2023

pelson commented Jan 26, 2023

uranusjr commented Jan 31, 2023

		: {file}`%VIRTUAL_ENV%\\pip.ini`
		: {file}`\{sys.prefix\}\\pip.ini`

		: {file}`$VIRTUAL_ENV/pip.conf`
		: {file}`\{sys.prefix\}/pip.conf`

Implement config file location 'base' to config all environments of an interpreter #11487

Implement config file location 'base' to config all environments of an interpreter #11487

Conversation

pelson commented Oct 5, 2022

pradyunsg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelson Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelson commented Oct 6, 2022

uranusjr commented Nov 16, 2022 • edited Loading

pfmoore commented Nov 16, 2022

uranusjr commented Nov 17, 2022

pelson commented Nov 17, 2022

pfmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfmoore commented Nov 17, 2022

pradyunsg commented Jan 25, 2023

pradyunsg commented Jan 25, 2023

pelson commented Jan 26, 2023

uranusjr commented Jan 31, 2023

pelson Oct 6, 2022 •

edited

Loading

uranusjr commented Nov 16, 2022 •

edited

Loading