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 environment layering for uv run --with #3447

Merged
merged 1 commit into from
May 8, 2024
Merged

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented May 8, 2024

Summary

This PR takes a different approach to --with for uv run. Now, instead of merging the requirements and re-resolving, we have two phases: (1) sync the workspace requirements to the workspace environment; then (2) sync the ephemeral --with requirements to an ephemeral environment. The two environments are then layered by setting the PATH and PYTHONPATH variables appropriately.

I think this approach simplifies a few things:

  1. Once we have a lockfile, the semantics are much clearer, and we can actually reuse it for the workspace. If we had to add arbitrary dependencies via --with, then it's not really clear how the lockfile would/should behave.
  2. Caching becomes simpler, because we can just cache the ephemeral environment based on the requirements.

The current version of this PR loses a few behaviors though that I need to restore:

  • --python support -- but I'm not yet sure how this is supposed to behave within projects? It's also left unclear in uv sync and uv lock.
  • The "reuse the workspace environment if it already satisfies the ephemeral requirements" behavior.

Closes #3411.

Comment on lines 73 to 74
// Sync the workspace requirements.
Some(sync_environment(venv, &workspace_requirements, preview, cache, printer).await?)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just throwing this out here to think about... wouldn't it be annoying if we exposed a default environment at .venv and every time you used uv run any manual changes you had made were undone?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, though this doesn't uninstall anything, it just makes sure it's in-sync with the (in-the-future) lockfile. Does that seem right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah we'll need to be careful with "in sync" because sync implies an exact match (no extra packages) to me. Seems ideal not to remove packages unless requested? Like.. uv sync --strict?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in uv run, not uv sync. For now, neither one is removing packages, but I don’t feel strongly about the behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that, I'm talking more generally.

uv run should not probably not remove extraneous packages (and I understand that is the current behavior).

@charliermarsh charliermarsh force-pushed the charlie/layer branch 3 times, most recently from 0930363 to 2b1e115 Compare May 8, 2024 02:35
Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know how resource discovery interacts with PYTHONPATH, e.g. for finding jupyter kernels?

iter::once(python_env.scripts().to_path_buf()).chain(env::split_paths(&path));
env::join_paths(python_env_path)?
// If necessary, create an environment for the ephemeral requirements.
let tmpdir;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's curious, how is that implemented in rust, does this insert a conditional drop?

Base automatically changed from charlie/sync to main May 8, 2024 14:51
@charliermarsh charliermarsh force-pushed the charlie/layer branch 3 times, most recently from f236a64 to 9085bba Compare May 8, 2024 19:59
@charliermarsh charliermarsh enabled auto-merge (squash) May 8, 2024 20:00
@charliermarsh charliermarsh merged commit fa43288 into main May 8, 2024
43 checks passed
@charliermarsh charliermarsh deleted the charlie/layer branch May 8, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Experimental behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants