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

Use base_prefix to detect virtual env #2566

Merged
merged 2 commits into from
Jan 10, 2023

Conversation

eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented Dec 28, 2022

See the discussion in #1860.

Detect presence of a virtual enviroment using sys.prefix != sys.base_prefix. This is more robust than a check on the environment variable VIRTUAL_ENV.

Fixes #1052

@j9ac9k @pijyoi

A minimal example (using Windows):

  1. Create a virtual environment using virtualenv
  2. Activate the environment
  3. Run python -c "import os; print(os.environ.get('VIRTUAL_ENV'))". The output shows the location of the virtualenv
  4. Install Spyder (pip install spyder)
  5. Start spyder (by typing the command spyder). In the Spyder console window run import sys; print(sys.prefix). The output is the location of the virtual environment, which shows the console session in spyder is indeed running the virtual environment.
  6. In the same console window of spyder run import os; print(os.environ.get('VIRTUAL_ENV')). The output is None. So the original check does not work in this case
  7. In the same console window run import sys; sys.prefix != sys.base_prefix. The output is True

Other Tasks

Bump Dependency Versions

Files that need updates

Confirm the following files have been either updated or there has been a determination that no update is needed.

  • README.md
  • setup.py
  • tox.ini
  • .github/workflows/main.yml and associated requirements.txt and conda environemt.yml files
  • pyproject.toml
  • binder/requirements.txt
Pre-Release Checklist

Pre Release Checklist

  • Update version info in __init__.py
  • Update CHANGELOG primarily using contents from automated changelog generation in GitHub release page
  • Have git tag in the format of pyqtgraph-
Post-Release Checklist

Steps To Complete

  • Append .dev0 to __version__ in __init__.py
  • Announce on mail list
  • Announce on Twitter

@eendebakpt eendebakpt changed the title Draft: Use base_prefix to detect virtual env Use base_prefix to detect virtual env Dec 28, 2022
@pijyoi
Copy link
Contributor

pijyoi commented Dec 28, 2022

Is this not an issue with Spyder messing with the environment?
https://github.com/spyder-ide/spyder/blob/master/spyder/plugins/ipythonconsole/utils/kernelspec.py#L173

@pijyoi
Copy link
Contributor

pijyoi commented Dec 29, 2022

IPython makes use of the VIRTUAL_ENV environment variable and Spyder wants to fool IPython.
https://github.com/ipython/ipython/blob/27ca9e2e3f8c9220a8b92938e11157a77c2c4893/IPython/core/interactiveshell.py#L845

Maybe the fix should be at Spyder?

@eendebakpt
Copy link
Contributor Author

IPython makes use of the VIRTUAL_ENV environment variable and Spyder wants to fool IPython. https://github.com/ipython/ipython/blob/27ca9e2e3f8c9220a8b92938e11157a77c2c4893/IPython/core/interactiveshell.py#L845

Maybe the fix should be at Spyder?

@pijyoi Thanks for the pointer, that gives good information. I do not know whether we should fix this at the spyder or pyqtgraph side. I created spyder-ide/spyder#20273 to get input from the spyder devs.

The PR here is simple, so if there are no downsides and there is no easy solution at the spyder side I would be in favor of this option.

@j9ac9k
Copy link
Member

j9ac9k commented Jan 7, 2023

Hi @eendebakpt

I'm ok merging this change, but I would request you add a comment with a link to the issue in spyder, so that future people maintaining the library don't have to scratch their heads as to why this change was made, and in turn can track if the other issue was ever fixed (and if it is fixed/addressed, when can this patch be removed from pyqtgraph).

EDIT: btw thanks for creating the issue w/ the Spyder repo; hopefully there will be an explanation there, ccordoba12 (intentionally not tagging to minimize notification spam) is a friend of the project :)

@eendebakpt
Copy link
Contributor Author

Hi @eendebakpt

I'm ok merging this change, but I would request you add a comment with a link to the issue in spyder, so that future people maintaining the library don't have to scratch their heads as to why this change was made, and in turn can track if the other issue was ever fixed (and if it is fixed/addressed, when can this patch be removed from pyqtgraph).

EDIT: btw thanks for creating the issue w/ the Spyder repo; hopefully there will be an explanation there, ccordoba12 (intentionally not tagging to minimize notification spam) is a friend of the project :)

@j9ac9k No response on the spyder github issue yet, but I added links to the issues in the comments.

@j9ac9k
Copy link
Member

j9ac9k commented Jan 9, 2023

Hi @eendebakpt
I'm ok merging this change, but I would request you add a comment with a link to the issue in spyder, so that future people maintaining the library don't have to scratch their heads as to why this change was made, and in turn can track if the other issue was ever fixed (and if it is fixed/addressed, when can this patch be removed from pyqtgraph).
EDIT: btw thanks for creating the issue w/ the Spyder repo; hopefully there will be an explanation there, ccordoba12 (intentionally not tagging to minimize notification spam) is a friend of the project :)

@j9ac9k No response on the spyder github issue yet, but I added links to the issues in the comments.

Thanks, that's perfect!

@j9ac9k j9ac9k merged commit a11d109 into pyqtgraph:master Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiprocess does not work in a venv
3 participants