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 workspace members from sibling directories #4499

Closed
wants to merge 1 commit into from

Conversation

idlsoft
Copy link
Contributor

@idlsoft idlsoft commented Jun 25, 2024

Summary

Shared libraries for independent projects.

Let's say I have the following layout:

src_root
├── app1
├── app2
├── shared_lib
└── shared_corelib

app1 and app2 never have to coexist in the same virtualenv, their dependencies are independent
and allowed to conflict with each other. shared_lib and shared_corelib, being libraries, have to play nice with both of them.

I want to use worskpaces to define the following direct dependencies:

  • app1->shared_lib
  • app2->shared_lib
  • shared_lib->shared_corelib

But if I simply create a workspace in src_root it'll try to create a shared lock and shared virtualenv for all four projects.
And if I create a workspace in either app1 or app2, they won't let me include shared_lib because it's in a sibling directory, and workspaces require a single root.

At my previous job I've implemented a build, that assumed this kind of scenario, and the workspace part was only responsible for locating members, and storing shared settings like default dependency versions, constraints, etc. This has the downside of having many virtualenvs so may not be for everyone, but I think it's a valid case.

I don't know if there are any future plans to support this in uv, perhaps by introducing the notion of a "leaf" member?

In the meantime, the scenario could be supported if workspaces were allowed to include sibling directories.
This is what this PR does.

Test Plan

Unit test

@zanieb zanieb requested a review from konstin June 25, 2024 01:21
@idlsoft idlsoft force-pushed the sibling_ws_members branch 3 times, most recently from 64b4c1e to 1fd8c2f Compare June 25, 2024 03:33
@idlsoft idlsoft marked this pull request as ready for review June 25, 2024 03:43
@idlsoft idlsoft marked this pull request as draft June 25, 2024 04:23
@idlsoft idlsoft force-pushed the sibling_ws_members branch 3 times, most recently from 959c662 to 6da8f64 Compare June 25, 2024 16:53
@idlsoft idlsoft marked this pull request as ready for review June 25, 2024 16:59
@konstin
Copy link
Member

konstin commented Jun 25, 2024

The reason we've been enforcing that package need to be below the workspace root is for dependencies into other workspace. Say we have a workspace:

A
|- B
|- C

and another

D
|- E
|- F

and dependencies B -> E and E -> F. With our current design, E can say F = { workspace = true } and we can figure out the correct paths for B -> E -> F by walking up the tree. This feature is also supported by cargo.

For your use case, can you use a regular editable path dependency instead of a workspace?

@idlsoft
Copy link
Contributor Author

idlsoft commented Jun 25, 2024

and dependencies B -> E and E -> F. With our current design, E can say F = { workspace = true } and we can figure out the correct paths for B -> E -> F by walking up the tree. This feature is also supported by cargo.

In this example B would have to make sure that both E and F are included in its workspace definition.
cargo is a bit different, because it doesn't have to create a shared virtualenv, and independent crates are allowed to have conflicting dependencies. You also can't reference a library you don't depend on, just because it happens to be in the same workspace.

I know this is not the most elegant solution.
Here is, I think, a nicer one:

There is a method in workspace.rs:

pub fn with_current_project(self, package_name: PackageName) -> Option<ProjectWorkspace> 

It could read the pyproject.toml of package_name and look for something like

[tool.uv]
top-level = true

If that's set it would filter out members that aren't a direct or transitive dependency of package_name, and make package_name the workspace root, thus driving the location of uv.lock, .venv, overrides etc.

For your use case, can you use a regular editable path dependency instead of a workspace?

I tried that a while ago, but uv and hatch were fighting each other, and the only way I got it to work was with this, another bandaid.

I'm not a big fan of either of these PRs, ideally I'd like to see workspaces support independent members natively.
If what I described above doesn't conflict with any ongoing work or future plans, I can try implementing it.

@idlsoft idlsoft force-pushed the sibling_ws_members branch 3 times, most recently from 48e168e to 2013de9 Compare June 25, 2024 23:51
@idlsoft
Copy link
Contributor Author

idlsoft commented Jun 26, 2024

In this example B would have to make sure that both E and F are included in its workspace definition.

After looking at the source a bit more I realize this isn't quite right.
Projects need to be able to lookup for their workspaces.

I updated the sample workspace to allow dependencies between libraries.

@konstin
Copy link
Member

konstin commented Jun 27, 2024

I am aware of the current problems when trying to combine relative paths, PEP 621/pyproject.toml and a build backend, but i don't think workspaces are the right tool to solve. Workspaces are intended for cases where there's a single project consisting of multiple package, to give each package an individual dependency specification and - if wanted - configuration in pyproject.toml. They don't work well as a tool for sharing a library between two applications.

@idlsoft
Copy link
Contributor Author

idlsoft commented Jun 27, 2024

They don't work well as a tool for sharing a library between two applications.

That's true, but I think with a few tweaks they could provide a clean solution for both.

This particular change is a few lines only, doesn't break any existing code, and already makes the scenario possible.

I also opened #4574, proposing a friendlier solution. It'll require a bit more coding, but I don't think it's going to be too intrusive either.

@idlsoft
Copy link
Contributor Author

idlsoft commented Jun 28, 2024

The new PR provides explicit support for the use-case.
It's a lot more straightforward from an end-user perspective, and interestingly didn't require the change from .strip_prefix to relative_to.

@konstin
Copy link
Member

konstin commented Jun 28, 2024

Currently it doesn't break any existing code, but with #3943 in place and people starting to have e.g. git dependencies into a package in a workspace, i'm afraid this change would cause some breakage.

If you could write-up an issue about your problems using hatch and uv together with relative path dependencies, i can have look and we can figure out a way to make this work. Starting off with a specific problem and discussing possible solutions and existing constraints we can much better solve your use case and avoid rejecting PRs that you've put effort into.

@idlsoft
Copy link
Contributor Author

idlsoft commented Jun 28, 2024

with #3943 in place and people starting to have e.g. git dependencies into a package in a workspace, i'm afraid this change would cause some breakage.

I believe this PR kinda close the 1st bullet point from #3943, while #4610 addresses the same underlying issue differently, within a single workspace, with no extra toml to write.

If you could write-up an issue about your problems using hatch and uv together with relative path dependencies, i can have look and we can figure out a way to make this work.

I'll try to come up with an example. But from what I remember, it was something like

  • uv doesn't allow lib @ ../lib and didn't like {root:uri}, but supports ${PROJECT_ROOT}
  • hatch doesn't like ${PROJECT_ROOT} but is happy with lib @ ../lib and its own {root:uri}

Starting off with a specific problem and discussing possible solutions and existing constraints we can much better solve your use case and avoid rejecting PRs that you've put effort into.

I don't mean to overwhelm you with PRs, but figuring out a solution is fun regardless of whether it gets merged or not.

The problem I'm trying to solve is described in #4574.
Multiple apps, using multiple libraries directly, as editable dependencies, coexisting even if their dependencies aren't compatible.
I believe workspaces can provide an elegant way of solving it, and it'll look familiar to anyone who's ever used cargo, or gradle.

@idlsoft
Copy link
Contributor Author

idlsoft commented Jul 18, 2024

The use-case is supported using

[tool.uv.sources]
shared_lib = { path = "../shared_lib", editable = true}

@idlsoft idlsoft closed this Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants