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

Allow --force to overwrite existing virtualenv #2548

Merged
merged 1 commit into from
May 1, 2024
Merged

Conversation

charliermarsh
Copy link
Member

Summary

Closes #2529.

Test Plan

  • mkdir .venv
  • touch .venv/foo
  • cargo run venv (ensure failure)
  • cargo run venv --force (ensure success)
  • cargo run venv --force (ensure success again)

@charliermarsh charliermarsh added enhancement New feature or request cli Related to the command line interface labels Mar 19, 2024
@charliermarsh charliermarsh changed the title Allow --force to overwrite existing virtualenv Allow --force to overwrite existing virtualenv Mar 19, 2024
@charliermarsh charliermarsh marked this pull request as ready for review March 19, 2024 19:02
@zanieb
Copy link
Member

zanieb commented Mar 19, 2024

Should we resolve #1472 too? I imagined gating that behavior with --force when we added it.

I presume this closes #1863 as well.

Supersedes #1824

@zanieb
Copy link
Member

zanieb commented Mar 19, 2024

I do make use of uv venv clearing the environment all the time but it seems more user friendly (and safe) to require -f.

@charliermarsh
Copy link
Member Author

Before I respond to that, I should note that --force is different than what you're describing. --force does not clear the folder. That was the specific request in the linked issue.

@zanieb
Copy link
Member

zanieb commented Mar 19, 2024

Ah sorry for the ambiguous language, I mean "clear the environment" as-in reset the virtual environment to an initial state not "clear the entire folder".

e.g. uv venv foo fails if foo exists at all and uv venv foo --force updates foo to be a fresh virtual environment without removing unrelated files. I don't see a need for a way to rm -rf foo via a flag.

@charliermarsh
Copy link
Member Author

That makes sense. I guess I'm not totally sold on changing the current overwrite behavior... I have a hunch that it's a difference in the API, and that leads to initial confusion, but that it's actually not a bad behavior.

@charliermarsh
Copy link
Member Author

Relatedly, --clear would be the comparable virtualenv flag for this.

@T-256
Copy link
Contributor

T-256 commented Mar 19, 2024

Should we resolve #1472 too?

Fwiw, I also think it's better to resolve #1472 first. Actually I expected to get error on continuous uv venv.
Then we can consider --force-overwrite and --force-recreate flags.

@charliermarsh
Copy link
Member Author

I just can't think of a time when I've ever run virtualenv and didn't want it to reset the environment.

@zanieb
Copy link
Member

zanieb commented Mar 20, 2024

I think it would happen when you didn't realize that there was an existing environment... which would be bad if you weren't constructing your environment from lockfiles (which is fairly common for people other than us).

@gaborbernat
Copy link
Contributor

Any updates on this?

@gaborbernat
Copy link
Contributor

@zanieb @charliermarsh did this get lost?

@charliermarsh
Copy link
Member Author

I think we just lost steam because it started to get entangled with the discussion around whether we should change the uv venv behavior for existing virtualenvs in general.

@zanieb
Copy link
Member

zanieb commented Apr 17, 2024

And what the flags should be for these various behaviors

@zanieb
Copy link
Member

zanieb commented Apr 17, 2024

Concretely, I'd be okay with:

--force: Create an environment without checking for unknown files
--clear / --reset: When creating, ensure site-packages is empty. Implies --force.

I don't see any reason to remove unknown files on --clear. That feels like different functionality.

@charliermarsh
Copy link
Member Author

So would the behavior without any flags change? (Does --clear do an rm -rf .venv?)

@zanieb
Copy link
Member

zanieb commented Apr 17, 2024

I think the existing behavior should change to fail if .venv exists without one of these flags and --clear should not imply rm -rf .venv — that seems like something you should do manually if you want to remove things other than packages?

@charliermarsh
Copy link
Member Author

I don’t understand what clear does then. And clear would then differ from virtualenv despite having the same name. What does it do? Removes specific folders?

@zanieb
Copy link
Member

zanieb commented Apr 17, 2024

And clear would then differ from virtualenv despite having the same name.

Hence suggesting --reset instead.

Isn't what I wrote concrete?

When creating, ensure site-packages is empty
I don't see any reason to remove unknown files on --clear

I expect it to remove all the packages and reset the virtual environment files (i.e. that it would create) not delete every file in the directory.

@zanieb
Copy link
Member

zanieb commented Apr 17, 2024

Not tied to this proposal, just putting something concrete forward so we can make progress on it.

@charliermarsh
Copy link
Member Author

I think I'm just trying to understand the motivation for keeping some of the content in the virtualenv. When would that be needed?

@zanieb
Copy link
Member

zanieb commented Apr 17, 2024

I'm not sure, for whatever weird thing is going on in #2529?

Perhaps what I'm thinking is... it feels hard for a user to just clear the packages themselves but very easy for a user to delete the whole directory if they want.

@T-256
Copy link
Contributor

T-256 commented Apr 18, 2024

I think the existing behavior should change to fail if .venv exists without one of these flags

👍

I think I'm just trying to understand the motivation for keeping some of the content in the virtualenv. When would that be needed?

It's useful when an external tool adds its specified files to .venv directory. when use --clear/--reset it's better avoid using external tool again to create those files/folders. I don't know real world example for such case, but I think this is one of possible scenarios.

@charliermarsh
Copy link
Member Author

@zanieb - thought I thought #2529 was like: they create a new directory, they add some files, then they want to add a virtualenv to it (rather than: they have an existing virtualenv that they want to clear).

@gaborbernat
Copy link
Contributor

@zanieb - thought I thought #2529 was like: they create a new directory, they add some files, then they want to add a virtualenv to it (rather than: they have an existing virtualenv that they want to clear).

Yes correct, this is my use case.

@zanieb
Copy link
Member

zanieb commented Apr 18, 2024

@charliermarsh so yeah once you're in that situation how do you clear the packages from the virtual environment in the folder?

@charliermarsh
Copy link
Member Author

I don't think they reuse the directory in that way, though. It sounds like they create a new directory, seed it, use it for a virtualenv, then move on. I imagine that they then repeat that entire process when they need a new virtualenv. So, I'm not sure it's worth designing around and supporting that workflow when we don't have a clear motivating use-case.

@zanieb
Copy link
Member

zanieb commented Apr 22, 2024

I think about it like this:

I use uv venv to clear an environment all the time. It's nice to reset the installed packages in a single command. We should provide a flag to toggle behavior when the directory exists already. I think this behavior should not be opt-out, but instead opt-in — manually managed environments are rarely disposable for real users. I would find it surprising that it performs rm -rf .venv and deletes files unrelated to the virtual environment itself. I cannot trivially clear packages from a virtual environment manually, as it can vary per platform. I can, however, trivially delete the folder. I'd like uv to take care of the non-trivial part of this workflow. I admit there is still value to performing the trivial part, i.e. it's a single invocation instead of rm -rf .venv && uv venv. It seems rare that you would need to clean up other state in the directory though, in which case I think the two command invocation is clearer anyway.

@charliermarsh
Copy link
Member Author

🤷 IDK, personally I would find it surprising if --clear or --reset left some random files in the virtualenv. I think it's likely that we eventually hear from a user that's confused that --clear or --reset doesn't fully clear or reset the virtual environment. We'll may also see some bug eventually around files that we can't track in our implementation but that "should" be cleared (e.g., what if virtualenv or python -m venv writes a file to the top-level, and a user runs --clear on one of those virtualenvs?). It feels like a hard behavior to explain and implement reliably for a use-case we don't know about.

If we can't agree, then should we at least move forward with --force as-described, that allows you to create a virtualenv ignoring any existing files?

@zanieb
Copy link
Member

zanieb commented Apr 30, 2024

🤷‍♀️ okay let's just go forward with both --force and --clear

@konstin
Copy link
Member

konstin commented Apr 30, 2024

I like the current behavior, it encourages treating venvs as ephemeral

@zanieb
Copy link
Member

zanieb commented Apr 30, 2024

I feel quite strongly that our current behavior is nice for us but doesn't make sense for the average user. Virtual environments are going to be ephemeral and abstract in our future workflows, we don't need to enforce this idea by making it easy to delete your environment when using a plumbing command.

@charliermarsh
Copy link
Member Author

Okay, let's proceed with --force for now. We can always decide on --clear or --reset after.

Copy link

codspeed-hq bot commented May 1, 2024

CodSpeed Performance Report

Merging #2548 will not alter performance

Comparing charlie/force (8f65128) with main (630d3fd)

Summary

✅ 12 untouched benchmarks

Copy link

codspeed-hq bot commented May 1, 2024

CodSpeed Performance Report

Merging #2548 will not alter performance

Comparing charlie/force (da4d103) with main (630d3fd)

Summary

✅ 12 untouched benchmarks

@charliermarsh charliermarsh enabled auto-merge (squash) May 1, 2024 16:10
Copy link

codspeed-hq bot commented May 1, 2024

CodSpeed Performance Report

Merging #2548 will not alter performance

Comparing charlie/force (540aab1) with main (630d3fd)

Summary

✅ 12 untouched benchmarks

@charliermarsh charliermarsh merged commit 614c073 into main May 1, 2024
43 checks passed
@charliermarsh charliermarsh deleted the charlie/force branch May 1, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command line interface enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow creating a virtual environment in a folder with existing files
5 participants