PR: Use local environment variables and Add option to import PYTHONPATH

[mrclary]

Description of Changes

The macOS application now imports the user's system environment variables into its os.environ so that they are passed to IPython consoles.

The PYTHONPATH Manager now has a a button to import the PYTHONPATH from the user's system environment (os.environ['PYTHONPATH']). This is useful for all platforms that have PYTHONPATH defined outside Spyder: either the Windows registry (Windows), or in the bash profile stack (Linux and macOS). This does not replace the functionality to export the PYTHONPATH Manager list to the Windows registry.

The PYTHONPATH Manager widget is now enforced to be a single instance.

Completions through PyLS now honor the path list in PYTHONPATH Manager, which is what should be expected.

With this PR, the user now has complete control over the PYTHONPATH used for IPython consoles via the PYTHONPATH Manager with the convenience of importing PYTHONPATH from the user's system environment variable. This PR harmonizes completions between the Editor and IPython console and across all launch mechanisms and platforms.

Issue(s) Resolved

Fixes #14843
Fixes #14809

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:
@mrclary

Comments

This is a better implementation of the basic idea in #14918. In the discussion of that PR, I had proposed the idea of creating an Environment Variables plugin. While this is still a good idea, I think that creating a new plugin will take more time and effort than I currently have and the basic functionality introduced in this PR is highly valuable to myself and colleagues. Therefore, I think this intermediate implementation is prudent.

[pep8speaks]

Hello @mrclary! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file spyder/app/mainwindow.py:

Line 1758:1: W293 blank line contains whitespace

• In the file spyder/utils/icon_manager.py:

Line 212:80: E501 line too long (86 > 79 characters)

Comment last updated at 2022-02-20 02:43:07 UTC

[mrclary]

PYTHONPATH Manager new widget appearance.

• Addition of "Import" button
• Changed icon for "Synchronize..." button (Windows only)

[mrclary]

I need to check the behavior of running scripts in external terminals; see #16200

[ccordoba12]

@mrclary, I disagree with most on this PR. Unless you revert almost all changes and focus on the ones related to our PYTHNONPATH manager, I won't accept it, sorry

The macOS application now imports the user's system environment variables into its os.environ so that they are passed to IPython consoles.

I didn't understand how this is achieved very well. But if it involves grabbing os.environ and passing it to the kernel, I disagree with it.

We need to add a way for users to be aware we're doing that, so they can enable/disable it if they want. Also, this could introduce odd crashes, so it should be opt-in, not opt-out.

Completions through PyLS now honor the path list in PYTHONPATH Manager, which is what should be expected.

We absolutely don't want that. Passing PYTHONPATH to the server and transport layers causes odd crashes (just add an empty string.py to a directory in PYTHONPATH to see), and it's totally unnecessary because we have the extra_paths that we can set in the server for it. As a matter of fact. that's what we're already doing for projects and it's working fine:

With this PR, the user now has complete control over the PYTHONPATH used for IPython consoles via the PYTHONPATH Manager with the convenience of importing PYTHONPATH from the user's system environment variable.

That's already working fine:

So, in my opinion, the only thing that's necessary is adding the Import button to our manager, so that an external PYTHONPATH can be added to it.

This PR harmonizes completions between the Editor and IPython console and across all launch mechanisms and platforms.

We don't want and don't need that. Each of those external processes works differently and there's no need to uniformize things for the sake of correctness.

[ccordoba12]

The PYTHONPATH Manager now has a a button to import the PYTHONPATH from the user's system environment (os.environ['PYTHONPATH']). This is useful for all platforms that have PYTHONPATH defined outside Spyder: either the Windows registry (Windows), or in the bash profile stack (Linux and macOS). This does not replace the functionality to export the PYTHONPATH Manager list to the Windows registry.

The PYTHONPATH Manager widget is now enforced to be a single instance.

Of course, these additions are nice and I agree with including them.

[mrclary]

The macOS application now imports the user's system environment variables into its os.environ so that they are passed to IPython consoles.

I didn't understand how this is achieved very well. But if it involves grabbing os.environ and passing it to the kernel, I disagree with it.

We need to add a way for users to be aware we're doing that, so they can enable/disable it if they want. Also, this could introduce odd crashes, so it should be opt-in, not opt-out.

This is already done for Spyder launched from the command line but not done for macOS standalone application. This PR only makes the standalone application behave the same as launching from commandline, i.e. Spyder's internal os.environ inherits a users environment variables. The user's environment variables are already passed to the IPython console (with some modification); this PR does not change that.

spyder/plugins/ipythonconsole/utils/kernelspec.py (126-130)

    def env(self):
        """Env vars for kernels"""
        default_interpreter = self.get_conf(
            'default', section='main_interpreter')
        env_vars = os.environ.copy()

If you want this to be an opt in option, then we would have to change the existing behavior.

Completions through PyLS now honor the path list in PYTHONPATH Manager, which is what should be expected.

We absolutely don't want that. Passing PYTHONPATH to the server and transport layers causes odd crashes (just add an empty string.py to a directory in PYTHONPATH to see), and it's totally unnecessary because we have the extra_paths that we can set in the server for it. As a matter of fact. that's what we're already doing for projects and it's working fine:

I agree that we do not want to pass PYTHONPATH to the server and transport layers. This PR does not do that. The completion search paths, as you say, are controlled by the extra_paths. The current issue is that, in fact, PYTHONPATH was being sent to PyLS. PyLS environment was not being set at all when run outside of DEV or pytests, but PyLS will default to use os.environ, which includes PYTHONPATH, when no environment is specified. See the changes I made to spyder/plugins/completion/providers/languageserver/client.py in the start_server method. Now, the server will inherit os.environ for all languages, but explicitly remove PYTHONPATH for python language. The problem this presented was that PYTHONPATH was being passed and added to extra_paths, so that completions erroneously included paths in Spyder's internal environment in addition to those specified by PYTHONPATH Manager.

With this PR, the user now has complete control over the PYTHONPATH used for IPython consoles via the PYTHONPATH Manager with the convenience of importing PYTHONPATH from the user's system environment variable.

That's already working fine:

For launching from conda, yes, the IPython console had both the user's system PYTHONPATH and that from PYTHONPATH Manager. But the macOS standalone application did not (see my comment above). However, this was not explicit, since the conda user would have to consult his bash profile chain plus the PYTHONPATH Manager to know what was going to the IPython console. Now, it is explicit; whatever is in PYTHONPATH Manager is what will be inherited in the IPython console. With the import button, the user now has explicit (opt in) control over exactly what paths go to the IPython console.

This PR harmonizes completions between the Editor and IPython console and across all launch mechanisms and platforms.

We don't want and don't need that. Each of those external processes works differently and there's no need to uniformize things for the sake of correctness.

I don't understand your objection here. Of course we want them to be the same. We want the completion paths in the Editor (via extra_paths) to be the same as that in the IPython console; having completions valid in one but not the other is confusing and misleading.

I actually think this PR does exactly what you would like, but maybe there is some confusion as to what was happening before and what I'm proposing to change. We should chat over zoom if we need to have a deeper conversation.

[mrclary]

Warning

The following comments are extensive; if you are looking for tl;dr, this isn't it 😁 . Because the issues are quite tedious to describe, and there has already been some misunderstanding, I make no apology for the verbosity. My goal is to explain as clearly as possible what this PR actually does and why. I've tried to be concise but thorough.

Clarifying the Issues

There are four main issues addressed by this PR

1. The PYTHONPATH Manager is not a single instance dialog.
2. When launching Spyder from a conda environment (traditional method) or the Windows standalone application, a user's local environment variables are inherited into Spyder's os.environ and subsequently passed to the IPython console. However, when launching Spyder as a standalone macOS application, this is not the case.
3. When launching Spyder from a conda environment (traditional method) and using the internal interpreter for the IPython console, completions in the Editor and the IPython console use search paths comprising the interpreter environment paths (site-packages, etc.), Spyder's PYTHONPATH Manager paths (PPM), and paths in the user's local PYTHONPATH environment variable (PPP). However, when using an external interpreter, the completion search paths for the Editor do not include the paths in the user's local PYTHONPATH environment variable. Thus there is an inconsistency between the completions in the Editor and the IPython console in this scenario.
4. When launching Spyder from a conda environment with bootstrap.py and using an external interpreter for the IPython console, completions in the Editor and the IPython console use search paths comprising the interpreter environment paths (site-packages, etc.) and Spyder's PYTHONPATH Manager paths, but do not include paths from the user's local PYTHONPATH environment variable. However, when using an internal interpreter, the completion search paths for the Editor also include the paths from the user's local PYTHONPATH environment variable. Thus there is an inconsistency between the completions in the Editor and the IPython console in this scenario. It should also be noted that while the user's local environment variables are inherited into Spyder's os.environ when launching with bootstrap.py, the paths in the user's PYTHONPATH is clobbered and replaced with spyder-kernels before sending it to the IPython console.

Note that there is also an inconsistency in completions behavior between the bootstrap and traditional conda launching methods. The following table summarizes the issues described by 2 - 4 above.

• "Launch" indicates the launch mechanism: (conda-env) $ spyder, (conda-env) $ python boostrap.py, or macOS standalone application.
• "Interpreter" indicates whether the IPython console interpreter is "Same as Spyder" (internal) or some other virtual environment (external).
• "Env variables" indicate whether the user's local environment variables are inherited into Spyder's os.environ. Note that in all cases, the IPython console will inherit Spyder's os.environ.
• "sys.path" indicates what paths are contained in the IPython console's sys.path. Note that "interpreter" here refers to the standard library paths for the installed packages in the interpreter's environment. These are automatically added by Python when the interpreter is started.
• "IPython comp" indicates the paths used for completions in the IPython console.
• "Editor comp" indicates the paths used for completions in the Editor

Launch	Interpreter	Env variables	sys.path	Ipython comp	Editor comp
conda	internal	inherited	interpreter + PPM + PPP	interpreter + PPM + PPP	interpreter + PPM + PPP
	external	inherited	interpreter + PPM + PPP	interpreter + PPM + PPP	interpreter + PPM
mac app	internal	not inherited	interpreter + PPM	interpreter + PPM	interpreter + PPM
	external	not inherited	interpreter + PPM	interpreter + PPM	interpreter + PPM
bootstrap	internal	inherited (PPP=spyder-kernels)	interpreter + PPM + spyder-kernels + python-lsp-server + qdarkstyle	interpreter + PPM	interpreter + PPM + PPP
	external	inherited (PPP=spyder-kernels)	interpreter + PPM + spyder-kernels	interpreter + PPM	interpreter + PPM

Analyzing Completions

Let's analyze the behavior of the completions search paths. First note that completions search paths are controlled via different mechanisms for the IPython console and the Editor.

• The Editor completion search paths are determined by the PYTHONPATH environment variable sent to PyLS (set in spyder/plugins/completion/providers/languageserver/client.py) and by the PYTHONPATH environment variable and extra_paths sent to jedi (as set in spyder/plugins/completion/providers/languageserver/provider.py).
• The IPython console completion search paths are just those in its sys.path, determined by the PYTHONPATH and SPY_PYTHONPATH environment variables passed to the console, as set in spyder/plugins/ipythonconsole/utils/kernelspec.py.

The Editor's Completion Paths

• conda + internal: this does include PPP because:
  • PyLS is passed an environment equivalent to None and thus defaults to Spyder's os.environ which contains PPP.
  • The env_vars passed to Jedi is None and thus defaults to the os.environ of its containing environment, which is PyLS's.
  • So PPP is passed from Spyder's os.environ to PyLS's os.environ to Jedi's os.environ. Note that these are in addition to extra_paths passed to Jedi.
• conda + external: this does not include PPP because:
  • PyLS is passed an environment equivalent to None, and thus defaults to Spyder's os.environ which contains PPP.
  • The env_vars passed to Jedi is explicitly set to Spyder's os.environ but then PYTHONPATH is explicitly removed.
  • Since env_vars passed to Jedi is not None, it will not default to PyLS's os.environ and PPP does not make it to the search paths.
• bootstrap + internal: this does include PPP because:
  • PyLS is passed only PYTHONPATH which is set to a "clean" version of Spyder's sys.path. This clean version starts with Spyder's sys.path (interpreter + PPM + PPP) and removes PPM. Note that none of Spyder's os.environ is passed, but PPP is still passed.
  • The env_vars passed to Jedi is None and thus defaults to the os.environ of its containing environment, which is PyLS's.
  • So PPP is passed from Spyder's sys.path to PyLS's os.environ to Jedi's os.environ. Note that the end result is the same as "conda + internal" but via Spyder's sys.path instead of os.environ.
• bootstrap + external: this does not include PPP because:
  • PyLS is passed only PYTHONPATH which is set to a "clean" version of Spyder's sys.path. This clean version starts with Spyder's sys.path (interpreter + PPM + PPP) and removes PPM. Note that non of Spyder's os.environ is passed, but PPP is still passed.
  • The env_vars passed to Jedi is explicitly set to Spyder's os.environ but then PYTHONPATH is explicitly removed.
  • Since env_vars passed to Jedi is not None, it will not default to PyLS's os.environ and PPP does not make it to the search paths.

IPython Console's Completion Paths

• conda + internal or external: these do include PPP because:
  • env_vars passed to IPython console is set to Spyder's os.environ which contains PPP.
• bootstrap + internal or external: these do not include PPP because:
  • env_vars passed to IPython console is set to Spyder's os.environ which contains PPP, but then PYTHONPATH therein is clobbered and replaced with spyder-kernels.

Proposed Solution

Issue 1 above does not present any controversy and is resolved by a single commit in this PR.

Issue 2 above does not present any controversy and is resolved by a single commit in this PR. Note that this is resolved by a change in the macOS app installer's setup.py and does not change any of Spyder's source code. However, this does change the completions behavior for the macOS application: originally there was no discrepancy between completions in the IPython console and Editor but now the macOS app will behave identically to the conda launch mechanism.

Now, regarding issues 3 and 4 above. I don't think it is controversial to say that the search paths for completions should:

• behave the same for both internal and external interpreters. I.e. interpreter installed package paths + PYTHONPATH Manager paths.
• be the same (or produce the same results), for a given interpreter, whether one is in the IPython console or the Editor. Of course, I'm only considering the scenario of a single console for now, not multiple consoles with different environments (that's a completely different issue).
  In addition, @ccordoba12 also asserts, and I agree, that PYTHONPATH should not be passed to PyLS for the Python language.

These can be accomplished by the following changes:

• Do not pass PYTHONPATH to PyLS for the Python language under any circumstance. This requires that we use a copy of os.environ with PYTHONPATH explicitly removed and the result explicitly passed to PyLS. This closes the back door through which PPP was being passed to Jedi inadvertently. This also is consistent with the fact that other languages may require environment variables.
• For the Jedi language provider, we can now safely pass env_vars=None, it will inherit os.environ from PyLS (which will never have PYTHONPATH), and the search paths are uniquely determined by the interpreter and extra_paths sent to Jedi.

With the above changes, however, now that PPP is no longer (inadvertently) passed to Jedi, completions for the Editor and IPython console will be different. We must now exercise control over PPP being passed to IPython console.

• os.environ is already explicitly used for the IPython console, so it is only required to explicitly remove PYTHONPATH.

However, there is the possible exception for "bootstrap + external". I believe the intention was to inject the local spyder-kernels path so that, under DEV, this would be found and used by external interpreters instead of spyder-kernels installed in the external environment. If this is the case, then a simple exception can be made here. But there may be reasons not to do this.

• Since this only affects bootstrap (DEV), then we are discussing the affects only seen by Spyder developers.
• If the developer needs to work with external interpreters, and needs to ensure that the "external-deps" version of spyder-kernels is used by the external interpreter, then it may be more prudent to install that version in that external environment (presumably in develop mode). Without installing it in the external interpreter environment, the developer may find unexpected behavior due to incompatible dependencies, i.e. trying to use spyder-kernels from "external-deps" (because it's found first on sys.path), when its dependencies in the external interpreter environment may not be compatible.

Now that we've removed all the inadvertent injections of PPP, we need to address how to use PPP conveniently and transparently. Adding the "import" mechanism to the PYTHONPATH Manager does just that.

• The user now transparently knows exactly what to expect from his completions: whatever is in the PYTHONPATH Manager
• The user's local PYTHONPATH paths can be imported to PYTHONPATH Manager, if desired. This only has to be done once since PYTHONPATH Manager paths persist across sessions.
• Throughout Spyder's source code, developers do not need to consider the contents of os.environ['PYTHONPATH'] when considering UX; everything should be in spyder_pythonpath configuration set by the PYTHONPATH Manager.
• The user now has more flexibility with respect to the imported PYTHONPATH. Previously, the user had no control over whether PPP was used, or which paths to include/exclude, but now has all the features of PYTHONPATH Manager (remove, deselect, change order) to bring to bear on those paths.

[mrclary]

@ccordoba12 @andfoy any chance of moving forward on this soon?

[mrclary]

I'm demoting this PR in favor of #17502 and #17503. These two PRs should be uncontroversial.
The added feature to the PYTHONPATH Manager to import the system PYTHONPATH will be introduced in a separate PR because it requires the comprehensive harmonizing of the treatment of PYTHONPATH, which was the controversial aspect of this PR.

[ccordoba12]

Ok, thanks for submitting those other PRs @mrclary!