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

fix(python): if already in an active virtual environment, use it (do not force/assume ".venv") #10159

Closed

Conversation

alexander-beedie
Copy link
Collaborator

@alexander-beedie alexander-beedie commented Jul 29, 2023

With all the bytecode introspection going on recently I've been switching between 3.9 and 3.11 Python venvs a lot, and noticed that we don't account for the possibility of being in a non-default venv in the Makefile; while in my .venv_39 environment I ran make requirements and realised that it was updating packages in the default .venv directory instead.

This PR makes a small update to the Makefile definition such that if we are already in a venv we will use it, otherwise we continue to default to the standard .venv.

@github-actions github-actions bot added fix Bug fix python Related to Python Polars labels Jul 29, 2023
@alexander-beedie alexander-beedie added the build Changes that affect the build system or external dependencies label Jul 29, 2023
@alexander-beedie alexander-beedie changed the title fix(python): if already in an active virtual environment, use it (do not assume ".venv") fix(python): if already in an active virtual environment, use it (do not force/assume ".venv") Jul 29, 2023
@stinodego
Copy link
Member

I'm not sure this will work as intended. It will still require a .venv folder on your disk, right?

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Jul 29, 2023

I'm not sure this will work as intended. It will still require a .venv folder on your disk, right?

Don't think so? The only behaviour change should be if you are already in a venv when you run make (which means you must have created one), in which case it sets VENV (and therefore also VENV_BIN) accordingly. Otherwise it should behave the same as before (creating .venv it if it doesn't exist, and using it if it does) 🤔

Logic is:

  • Not in a virtual env; runs as normal, using .venv (creating if necessary).
  • In a venv and it's the default .venv; runs as normal, doesn't create anything new.
  • In a venv and its not the default.venv; sets VENV & VENV_BIN to the active venv, ensuring any subsequent commands are correctly targeted at the active dir (and doesn't create anything new).

@cjackal
Copy link
Contributor

cjackal commented Jul 29, 2023

Looks to me like we still create a clean .venv if in a venv and .venv does not exist. And that .venv's python symlinks to original venv's python.

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Jul 29, 2023

Hmm, does the right thing if .venv exists; I'll move it out of the way and see what happens otherwise; about to go to sleep, so will have to wait for a day or two. In the meantime if anyone can spot a suitable improvement, please go ahead ;)

@stinodego
Copy link
Member

Basically, I think what you're trying to do is not supported by design. The Makefile is intended to work with .venv regardless of your active environment.

At some point, I played with the idea of having the Makefile be independent of the specific environment (basically remove the .venv requirement and leave it up to the user to create their own venv). But in the end, I don't think that's an improvement for most users. It would solve your issues in this case, though.

I think you're stuck with typing / hotkeying the actual commands for now instead of using make for your alternate environments. Unless you come up with a genius idea (wouldn't be the first time 😄 ).

@alexander-beedie
Copy link
Collaborator Author

Hmmm... ok, will stick this in Draft and poke at it some more 🤣

@alexander-beedie alexander-beedie marked this pull request as draft July 31, 2023 04:40
@alexander-beedie
Copy link
Collaborator Author

Will close out to get rid of PR clutter until I find the motivation to make this work...🤣

@alexander-beedie alexander-beedie deleted the fix-makefile-active-venv branch September 25, 2023 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Changes that affect the build system or external dependencies fix Bug fix python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants