[ty] Discover uv workspace roots - #25551
Conversation
Typing conformance resultsNo changes detected ✅Current numbersThe percentage of diagnostics emitted that were expected errors held steady at 96.84%. The percentage of expected errors that received a diagnostic held steady at 92.11%. The number of fully passing files held steady at 99/133. |
Memory usage reportMemory usage unchanged ✅ |
338b62d to
933d2df
Compare
|
| let uv_workspace_root = explicit_project_path | ||
| .is_none() | ||
| .then(|| uv::discover_workspace_root(&cwd)) | ||
| .flatten(); |
There was a problem hiding this comment.
I'm not sure if we want the project cli arg for ty to completely trump the uv signal or to get synthesized with it (longterm we want to "always" call uv for dependency graph stuff so avoiding calling it doesn't get us anywhere).
170a10c to
d440e53
Compare
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
I think we have to consider the precedence between uv and ty a bit more in detail.
I'd also prefer if we move this into ProjectMetadata::discover instead, to get a consistent experience when opening such a project in the LSP. This may also require changing the file watching to reload the project whenever a uv.toml changes.
06e876b to
0fccab3
Compare
|
There was a problem hiding this comment.
Thank you. I find it somewhat difficult to review this PR, because the summary contains very little detail and I don't know how the integration works on uv's side.
Would you mind extending the summary and explaining:
- The use case. What use cases are intentionally out of scope
- How is this called form uv. What argument does uv pass to ty? What was the reasoning behind this design.
- Document why we now call uv metadata eagerly. This is not that important to me, but i'm sure we'll forget in the future and having it document somewhere will be helpful
- What are the precedence rules and why did you define them as they are (I'm aware of most of them, but again, I think this will be valuable for us in the future)
Codex claims this finding
VIRTUAL_ENV incorrectlyoverrides an explicitUV_PROJECT_ENVIRONMENT, causing--syncand ty to use the wrong environment. uv normally gives the explicit project environment precedence. Reproduced with an import available only in the explicit environment. uv environment semantics
| // No explicitly configured project was found. Prefer the uv workspace over the closest | ||
| // plain `pyproject.toml`, if available. | ||
| let metadata = if let Some(uv_project) = uv_project { |
There was a problem hiding this comment.
This feels a bit weird, especially because it also applies if the uv integration isn't used?
What's the reason that we change the configuration precedence as part of this PR? Should this only apply if opted in with the environment variable?
e22b433 to
59d2ba6
Compare
| if !output.status.success() { | ||
| tracing::debug!( | ||
| "`uv workspace metadata` failed with status {}: {}", | ||
| output.status, | ||
| String::from_utf8_lossy(&output.stderr) | ||
| ); | ||
| return None; | ||
| } | ||
|
|
||
| match Self::from_metadata(&output.stdout, system) { | ||
| Ok(workspace) => Some(workspace), | ||
| Err(error) => { | ||
| tracing::debug!("Failed to use `uv workspace metadata` output: {error}"); | ||
| None | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Might want to surface these are errors humans see by default..? (still warnings)
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
This is an excellent summary. Thank you. It helped me a ton to understand the decision made in this PR.
There was a problem hiding this comment.
I suggest adding a link to the corresponding uv tests somewhere in this file (or in metadata/uv?. It helped me understand what uv does and what the expected behavior at the workspace level is. I know, it's not ideal, the link is likely to get outdated. But it at least gives me a pointer to where to look.
| /// Options derived from the uv workspace, with higher precedence than project and user-level | ||
| /// configuration. | ||
| #[cfg_attr(test, serde(skip_serializing_if = "Option::is_none"))] | ||
| uv_workspace_options: Option<Box<Options>>, | ||
|
|
There was a problem hiding this comment.
It wasn't entirely clear to me what uv_workspace_options are used for. It may be helpful to document that we only use those for requires_python and the interpreter path today
| .filter(|root| !path.starts_with(root)); | ||
|
|
||
| for project_root in path.ancestors() { | ||
| for project_root in path.ancestors().chain(external_uv_workspace_root) { |
There was a problem hiding this comment.
The chain here is a bit confusing.
I'm not sure if still possible, but I once split this method into two:
discover(does the outer loop)discover_in(discovers a config in a single directory)
I could help here.
Latest releases has updates to metadata and uv check that our tests want to rely on in #25551
Discover uv workspace roots and selected environments for uv check, preserve configuration-source attribution, and cover workspace, package, nested-member, active-environment, and script-path behavior.
053586d to
64ada2f
Compare
|
Pushed up some minor fixups based on review, and some test cleanups. |
4 participants
This introduces minimal support for acquiring
uv workspace metadatawhen, and only when, ty is invoked throughuv check, and packages/workspaces are being requested (and not e.g. --script, see below). All other invocations of metadata are "future work".uv checkRecapAs a recap,
uv checkon current main will:--packageto select a different one, and--all-packagesto select all of them)uv syncfor the selected packages, and to fetch the appropriate version of tyVIRTUAL_ENV=/path/to/.venv/UV=/path/to/uv.exeTY_UV=1ty check --exclude=packages/a/subpkg/ -- packages/a/ packages/b/aandbwere selected andsubpkgwas not, and an explicit exclude was introduced to prevent it from being implicitly analyzed as part ofa, under normal circumstances no exclude is needed, but any non-virtual workspace may need this kind of thing, and non-virtual workspaces are orthodox)The consequences of this are that
uv checkalready "just works" without this PR, however there are a few corner cases that are debatable.What This Changes
The following behaviours should only apply when TY_UV=1 mode is set, indicating we are being invoked through
uv check(if you set env-var we simply assume you areuv checkand GLHF if you're not; interactions with flags uv will never set are not guaranteed to make sense).uv workspace metadata --frozen --activein the cwd that uv told us to run in--frozenbecause we assume apriori thatuv checkalready ran lock+sync--activeto ensure we continue to use the VIRTUAL_ENV that uv told us to use (notably important for--isolated)--syncbecause the new default main uv behaviour foruv workspace metadatais to opportunistically emit environment metadata (python interpreter, package-to-module mappings) even if--syncisn't requested. We should not pass--syncin this case because uv has performed package/extra/group selection and we don't know what that was, and we are trying to avoid defining a protocol for forwarding that for the MVP.uv check --isolatedand there is no uv.lock then this will fail as the rerun with --frozen will not find any lock to use. This is fine for an MVP.conflictsthe package versions reported may be subtly incorrect. In general "how does ty operate in a world with conflicts" is "that's the neat part, it doesn't" for the forseeable future (uv check can maybe be made to work but how the heck does the LSP?)--no-projectand--no-syncare whacky nonsense I am refusing to put anymore thought into for an MVP.Future Work for
--watchIf TY_UV=1 mode is enabled,
--watchis forbidden (which is fine uv will never pass that). In the future it would be nice to haveuv check --watch, but this will require us to have a proper system for ty actually telling uv to resync with the proper package selection, and for uv metadata to have some way to report what members to watch.Future Work for
ty checkIt would be nice if
ty checkcould opportunistically detect that uv is available and auto-use TY_UV=1 mode. This is explicitly out of scope for the MVP as it causes endless UX arguments. I think under the current architecture it will probably "just" be ty instead opting to runuv workspace metadata --syncto sync the entire workspace (maybe with--isolatedif we don't want to clobber the "real" venv... also it would then remember the venv location and pass--activefor all subsequent invocations to keep reusing the same ephemeral venv).Note to self:
uv workspace metadata --isolatedexists but is fake because of the accursed "global deprecated --isolated flag that is different from the --isolated flag other commands have, so need to add that in the future if we decide we need it.Future Work For Scripts
In the current implementation
TY_UV=1mode is disabled if a path to a file is passed, as this should only happen ifuv check --scriptis run. This is a temporary measure to keep scope tight for an MVP.uv check --scriptalready works better thanty check path/to/script.pybecauseuv checkwill set VIRTUAL_ENV to the script's isolated venv -- the above just means we won't immediately have richer features TY_UV mode adds.In "package mode" where TY_UV=1 applies, if a package contains random pep723 scripts, ty will attempt to analyze them with the workspace's venv, which is wrong in the same way ty is wrong already. In a followup we will make ty check if a file is a script, and if so, not analyze it unless the CLI was passed an explicit path to exactly that file (indicating --script).
In a followup we can teach ty to run
uv workspace metadata --script=...when it finds a script and considers it in-scope to analyze it. Some fiddly corners around whether VIRTUAL_ENV should be cleared for such invocations, in general script venvs work kinda like ephemeral venvs but consistently resolved to the same path so you don't need to try super hard to be consistent, uv will figure it out.Future Work For LSP
LSP is out of scope for the MVP.
The rough idea for how it should work is that it will initially start up as it does now and opportunistically kick off
uv workspace metadata --sync (--isolated?)in the background. If that returns successfully, it's answers will then be respected and integrated in much the same way they are when invoked withuv check. This may cause mass invalidations but ty is fast so it's fine right? :)(The idea of this architecture is ty will give okish results early and fast and then as soon as uv is done "snap" into a richer form, in a similar way to how rust-analyzer can "snap to life" when
cargo checkcompletes.)After the first run it can then setup proper watchers and opportunistically invoke things like
uv workspace metadata --scriptor reinvokeuv workspace metadata --syncas needed. See the future work for scripts and watchers for details of that.Future Work for meow
Meow lints will opportunistically be enabled when the UvMetadata is attached to the ProjectMetadata and contains package-to-module maps. Meow and attaching more metadata will be implemented in a followup PR.