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

Initial workspace support #3705

Merged
merged 13 commits into from
May 28, 2024
Merged

Initial workspace support #3705

merged 13 commits into from
May 28, 2024

Conversation

konstin
Copy link
Member

@konstin konstin commented May 21, 2024

Add workspace support when using -r <path>/pyproject.toml or -e <path> in the pip interface. It is limited to all-editable static-metadata workspaces, and tests only include a single main workspace, ignoring path dependencies in another workspace. This can be considered the MVP for workspace support: You can create a workspace, you can install from it, but some options and conveniences are still missing. I'll file follow-up tickets (support in lockfiles, support path deps in other workspace, #3625)

There is also support in uv run, but we need #3700 first to properly support using different current projects in the bluejay interface, currently the resolution and therefore the lockfile depends on the current project. I'd do this change first (it's big enough already), then #3700, and then add workspace support properly to bluejay.

Fixes #3404

@konstin konstin force-pushed the konsti/workspaces branch from ed3ee3f to 203b2cf Compare May 21, 2024 17:17
Base automatically changed from konsti/discover-workspaces-without-using-them to main May 21, 2024 17:17
@konstin konstin force-pushed the konsti/workspaces branch 5 times, most recently from 8cc2fa7 to ec688f3 Compare May 22, 2024 12:37
@konstin konstin added the preview Experimental behavior label May 22, 2024
Copy link

codspeed-hq bot commented May 22, 2024

CodSpeed Performance Report

Merging #3705 will not alter performance

Comparing konsti/workspaces (273b3f6) with main (89cfece)

Summary

✅ 13 untouched benchmarks

@konstin konstin force-pushed the konsti/workspaces branch from ec688f3 to d9322e1 Compare May 22, 2024 19:10
@konstin konstin marked this pull request as ready for review May 22, 2024 19:12
@konstin konstin force-pushed the konsti/workspaces branch 2 times, most recently from 1281558 to 4e036f8 Compare May 23, 2024 11:26
konstin added a commit that referenced this pull request May 23, 2024
Make the error messages more consistent with the format use elsewhere. Split out from #3705
@konstin konstin force-pushed the konsti/workspaces branch from 4e036f8 to ee97dfd Compare May 23, 2024 11:52
konstin added a commit that referenced this pull request May 23, 2024
Make the error messages more consistent with the format use elsewhere.
Split out from #3705
@konstin konstin force-pushed the konsti/workspaces branch from ee97dfd to bd60f2a Compare May 23, 2024 15:12
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

The discovery logic looks good to me. Just a few comments.

crates/uv-requirements/src/pyproject.rs Outdated Show resolved Hide resolved
crates/uv-requirements/src/pyproject.rs Outdated Show resolved Hide resolved
crates/uv/tests/workspace.rs Show resolved Hide resolved
crates/uv-requirements/src/specification.rs Outdated Show resolved Hide resolved
@konstin konstin force-pushed the konsti/workspaces branch 2 times, most recently from 040bedf to 797cc9a Compare May 24, 2024 18:26
@charliermarsh
Copy link
Member

The workspace tests run fully offline using a directory index, this adds 648KB in the links directory. We could reduce this by switching back to setuptools or using even smaller deps (or just gut the existing ones). Those tests being offline makes them faster, easier to debug (no noise from the network requests) and avoids accidentally pulling a pypi package of the same name.

@konstin - sorry for going back-and-forth on this, but can we remove all the added links and just install from PyPI? I don't think we should change our test policy in a PR like this -- we pull directly for other tests, so it seems correct here. And checking-in binaries has its own downsides and risks.

@konstin konstin force-pushed the konsti/workspaces branch from 797cc9a to 88704d0 Compare May 26, 2024 21:40
Copy link
Member

Choose a reason for hiding this comment

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

Is this still necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed it

if !editable {
// TODO(konsti): Support this.
return Err(LoweringError::NonEditableWorkspaceDep);
}
Copy link
Member

Choose a reason for hiding this comment

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

When does this occur? I'm looking at this from the perspective of: what regressions could we ship that would accidentally impact users?

Copy link
Member Author

@konstin konstin May 27, 2024

Choose a reason for hiding this comment

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

This branch is only reachable when using uv.tool.sources (otherwise source is None), so it's preview gated

Copy link
Member

Choose a reason for hiding this comment

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

Can you please expand the TODO?

@konstin konstin force-pushed the konsti/workspaces branch from 66bba83 to bc920f1 Compare May 27, 2024 09:31
@@ -1,10 +1,41 @@
use std::collections::BTreeMap;
//! Collecting the requirements to compile, sync or install.
Copy link
Member

Choose a reason for hiding this comment

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

Should all of this logic instead be in source_tree.rs, like in the source tree resolver, instead of defined here?

Copy link
Member Author

Choose a reason for hiding this comment

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

What parts do you mean? If it's a larger refactoring, i'd do it in another PR

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we shouldn't be doing any dependency resolution in specification.rs and, instead, move all requirement collection into source_tree.rs.

Copy link
Member

Choose a reason for hiding this comment

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

(Not for this PR, I suppose.)

@konstin konstin enabled auto-merge (squash) May 28, 2024 07:33
@konstin konstin merged commit a89e146 into main May 28, 2024
46 checks passed
@konstin konstin deleted the konsti/workspaces branch May 28, 2024 07:41
@zanieb zanieb mentioned this pull request Jun 4, 2024
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.

Workspace support
3 participants