Skip to content

Use base_prefix to detect virtual env#2566

Merged
j9ac9k merged 2 commits intopyqtgraph:masterfrom
eendebakpt:remove_venv_v2
Jan 10, 2023
Merged

Use base_prefix to detect virtual env#2566
j9ac9k merged 2 commits intopyqtgraph:masterfrom
eendebakpt:remove_venv_v2

Conversation

@eendebakpt
Copy link
Copy Markdown
Contributor

@eendebakpt eendebakpt commented Dec 28, 2022 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
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 eendebakpt mentioned this pull request Dec 29, 2022
Open
@eendebakpt
Copy link
Copy Markdown
Contributor Author

eendebakpt 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?

@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
Copy Markdown
Member

j9ac9k commented Jan 7, 2023 •
edited
Loading

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
Copy Markdown
Contributor Author

eendebakpt 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.

@j9ac9k
Copy link
Copy Markdown
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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Multiprocess does not work in a venv

3 participants

@eendebakpt @pijyoi @j9ac9k