Skip to content

Workspace discovery #14308

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

Merged
merged 8 commits into from
Nov 15, 2024
Merged

Workspace discovery #14308

merged 8 commits into from
Nov 15, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Nov 13, 2024

Summary

Disocver workspaces and packages by searching for pyproject.toml. This PR loads the pyproject.toml files but it disregards all settings other than knot.workspace and project.name. I plan to add settings support in a separate PR (this one is already chunky).

  1. Traverse upwards in the path's ancestor chain and find the first pyproject.toml.
  2. If the pyproject.toml contains no knot.workspace table, then keep traversing the path's ancestor
    chain until we find one or reach the root.
  3. If we've found a workspace, then resolve the workspace's members and assert that the closest
    package (the first found package without a knot.workspace table is a member. If not, create
    a single package workspace for the closest package.
  4. If there's no pyproject.toml with a knot.workspace table, then create a single-package workspace.
  5. If no ancestor directory contains any pyproject.toml, create an ad-hoc workspace for path
    that consists of a single package and uses the default settings.

Differences to uv

I tried to keep the implementation close to uv's behavior. The main differences are:

  • Running Red Knot in a directory without any pyproject.toml (nor in any of its ancestor) is a valid use case. Red knot then creates a single package workspace with the default settings.
  • Red knot allows pyproject.toml without a project table.
  • uv errors when the current workspace is included in any outer workspace. However, I think it won't complain if the current workspace contains a member that is a workspace itself. This seems the wrong way around to me because what's important is that the current workspace has no nested workspace. Outer workspaces can be validated when running a command in them. This is why Red Knot only validates the workspace's members instead of discovering parent directories (https://github.com/astral-sh/uv/blob/e47e8fe9656b9f23891f9651f7bbc881f9e2bc7f/crates/uv-workspace/src/workspace.rs#L1193)
  • Red knot includes a member if it is explicitly listed in members but matches a exclude glob. E.g. members=["packages/*", "frontend"] and exclude=["frontend-*] still includes frontend if the directory is an exact match (not a glob-match). I think uv always excludes frontend which seems counterintuitive
  • The member discovery in Red Knot collects all member paths by running all members globs and then removing excluded members. uv does that too, but not for the "current package" where it uses is_excluded_from_workspace and is_included_in_workspace. I found using only globs easier. (https://github.com/astral-sh/uv/blob/e47e8fe9656b9f23891f9651f7bbc881f9e2bc7f/crates/uv-workspace/src/workspace.rs#L1133-L1147)

CC: @charliermarsh

Test Plan

Added unit tests and basic file watcher tests. We should add more file watcher tests but I struggled coming up with good examples because most test cases require some form of configuration support to make the change observable.

@MichaReiser MichaReiser force-pushed the micha/workspace-discovery branch 2 times, most recently from 3cd8c12 to 7522b98 Compare November 13, 2024 09:45
@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Nov 13, 2024
Copy link
Contributor

github-actions bot commented Nov 13, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser MichaReiser force-pushed the micha/workspace-discovery branch from 82a48ae to 1fc09bb Compare November 13, 2024 12:14
@MichaReiser
Copy link
Member Author

MichaReiser commented Nov 13, 2024

Oh damn you pre-commit. I'm just going to exclude that file over wasting time fighting pre-commit configuration.

@MichaReiser MichaReiser force-pushed the micha/workspace-discovery branch 6 times, most recently from 1a6640a to 35d3969 Compare November 13, 2024 16:25
@MichaReiser MichaReiser marked this pull request as ready for review November 13, 2024 16:28
@charliermarsh
Copy link
Member

I will review this today.

@charliermarsh
Copy link
Member

Oh I'm not requested here -- that's fine actually. Some feedback on the summary:

uv errors when the current workspace is included in any outer workspace. However, I think it won't complain if the current workspace contains a member that is a workspace itself. This seems the wrong way around to me because what's important is that the current workspace has no nested workspace. Outer workspaces can be validated when running a command in them.

No strong opinion though it'd be nice to change this in uv if you decide to do something different here.

Red knot includes a member if it is explicitly listed in members but matches a exclude glob. E.g. members=["packages/", "frontend"] and exclude=["frontend-] still includes frontend if the directory is an exact match (not a glob-match). I think uv always excludes frontend which seems counterintuitive

I would probably advise against doing this... It makes the logic more complex to implement, test, and document, and it deviates from uv without good reason? I would be surprised if this ever comes up. Even the example you gave isn't quite right given that the exclude contains a dash, if I'm understanding correctly ;)

Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

The workspace discovery looks great, thank you!

My main question about this PR (and most of my comments) are about how we intend to map these workspace settings to a module resolver search paths config (which is one of the most important outputs from workspace discovery.) I think currently in this PR there is a mismatch between workspace discovery and the module resolver's translation to search paths, which means we will do the wrong thing in a lot of cases, but I'm not sure if it's the module resolver or the workspace code that should change, because I don't understand what the intended semantics of SearchPathSettings::workspace_root are.

My review is mostly focused on these semantic and project structure questions; relying on Charlie's greater relevant experience from uv for the more filesystem and pyproject oriented stuff.

charliermarsh added a commit to astral-sh/uv that referenced this pull request Nov 15, 2024
… identical names (#9094)

## Summary

Align uv's workspace discovery with red knots (see
astral-sh/ruff#14308 (comment))

* Detect nested workspace inside the current workspace rather than
testing if the current workspace is a member of any outer workspace.
* Detect packages with identical names.

## Test Plan

I added two integration tests. I also back ported the tests to main to
verify that both these invalid workspaces aren't catched by uv today.
That makes this, technically, a breaking change but I would consider the
change a bug fix.

---------

Co-authored-by: Charlie Marsh <[email protected]>
@MichaReiser MichaReiser force-pushed the micha/workspace-discovery branch from 9bcdd7d to fd81fb1 Compare November 15, 2024 17:57
@MichaReiser MichaReiser merged commit 81e5830 into main Nov 15, 2024
20 checks passed
@MichaReiser MichaReiser deleted the micha/workspace-discovery branch November 15, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants