[ty] distinguish base conda from child conda #19990
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Gankra
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
Gankra
commented
Aug 19, 2025
Comment on lines
+866
to
+883
| /// To do this we create a program that has these 4 imports: | ||
| /// | ||
| /// ```python | ||
| /// from package1 import ActiveVenv | ||
| /// from package1 import ChildConda | ||
| /// from package1 import WorkingVenv | ||
| /// from package1 import BaseConda | ||
| /// ``` | ||
| /// | ||
| /// We then create 4 different copies of package1. Each copy defines all of these | ||
| /// classes... except the one that describes it. Therefore we know we got e.g. | ||
| /// the working venv if we get a diagnostic like this: | ||
| /// | ||
| /// ```text | ||
| /// Unresolved import | ||
| /// 4 | from package1 import WorkingVenv | ||
| /// | ^^^^^^^^^^^ | ||
| /// ``` |
There was a problem hiding this comment.
ngl this is so beautifully evil i might cry if y'all reject this
There was a problem hiding this comment.
This is fun (and clever). It took me a moment to realise how this all works. An alternative (I don't have a strong preference) which results in fewer diagnostics is to use something like this:
Each virtual environment defines a package1 that defines a single symbol, environment:
from typing import Final
environment: Final = "active_venv"The main file then becomes
from ty_extensions import static_assert
from package1 import environment
static_assert(environment == "active_venv", "Expected `package1` to resolve to the active environment")
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
cc @zanieb |
zanieb
reviewed
Aug 19, 2025
Comment on lines
+570
to
+573
| // These are the expected names for the base environment | ||
| if default_env != "base" && default_env != "root" { | ||
| return CondaEnvironmentKind::Child; | ||
| } |
There was a problem hiding this comment.
N.B. this isn't discussed in your commentary but we only allow these names for base environments. I don't know if people use other default environment names, but there's never been a complaint in uv.
There was a problem hiding this comment.
This seems straightforward to change if necessary
MichaReiser
approved these changes
Aug 20, 2025
Comment on lines
+866
to
+883
| /// To do this we create a program that has these 4 imports: | ||
| /// | ||
| /// ```python | ||
| /// from package1 import ActiveVenv | ||
| /// from package1 import ChildConda | ||
| /// from package1 import WorkingVenv | ||
| /// from package1 import BaseConda | ||
| /// ``` | ||
| /// | ||
| /// We then create 4 different copies of package1. Each copy defines all of these | ||
| /// classes... except the one that describes it. Therefore we know we got e.g. | ||
| /// the working venv if we get a diagnostic like this: | ||
| /// | ||
| /// ```text | ||
| /// Unresolved import | ||
| /// 4 | from package1 import WorkingVenv | ||
| /// | ^^^^^^^^^^^ | ||
| /// ``` |
There was a problem hiding this comment.
This is fun (and clever). It took me a moment to realise how this all works. An alternative (I don't have a strong preference) which results in fewer diagnostics is to use something like this:
Each virtual environment defines a package1 that defines a single symbol, environment:
from typing import Final
environment: Final = "active_venv"The main file then becomes
from ty_extensions import static_assert
from package1 import environment
static_assert(environment == "active_venv", "Expected `package1` to resolve to the active environment")
|
Appreciate the suggestion but the evil lives on :) (it avoids us having to change the body of main between subtests) |
dcreager
added a commit
that referenced
this pull request
Aug 21, 2025
* main: (29 commits) [ty] add docstrings to completions based on type (#20008) [`pyupgrade`] Avoid reporting `__future__` features as unnecessary when they are used (`UP010`) (#19769) [`flake8-use-pathlib`] Add fixes for `PTH102` and `PTH103` (#19514) [ty] correctly ignore field specifiers when not specified (#20002) `Option::unwrap` is now const (#20007) [ty] Re-arrange "list modules" implementation for Salsa caching [ty] Test "list modules" versus "resolve module" in every mdtest [ty] Wire up "list modules" API to make module completions work [ty] Tweak some completion tests [ty] Add "list modules" implementation [ty] Lightly expose `FileModule` and `NamespacePackage` fields [ty] Add some more helper routines to `ModulePath` [ty] Fix a bug when converting `ModulePath` to `ModuleName` [ty] Split out another constructor for `ModuleName` [ty] Add stub-file tests to existing module resolver [ty] Expose some routines in the module resolver [ty] Add more path helper functions [`flake8-annotations`] Remove unused import in example (`ANN401`) (#20000) [ty] distinguish base conda from child conda (#19990) [ty] Fix server hang (#19991) ...
Gankra
added a commit
that referenced
this pull request
Oct 3, 2025
<!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? --> ## Summary #19990 didn't completely fix the base vs. child conda environment distinction, since it detected slightly different behavior than what I usually see in conda. E.g., I see something like the following: ``` (didn't yet activate conda, but base is active) ➜ printenv | grep CONDA CONDA_PYTHON_EXE=/opt/anaconda3/bin/python CONDA_PREFIX=/opt/anaconda3 CONDA_DEFAULT_ENV=base CONDA_EXE=/opt/anaconda3/bin/conda CONDA_SHLVL=1 CONDA_PROMPT_MODIFIER=(base) (activating conda) ➜ conda activate test (test is an active conda environment) ❯ printenv | grep CONDA CONDA_PREFIX=/opt/anaconda3/envs/test CONDA_PYTHON_EXE=/opt/anaconda3/bin/python CONDA_SHLVL=2 CONDA_PREFIX_1=/opt/anaconda3 CONDA_DEFAULT_ENV=test CONDA_PROMPT_MODIFIER=(test) CONDA_EXE=/opt/anaconda3/bin/conda ``` But the current behavior looks for `CONDA_DEFAULT_ENV = basename(CONDA_PREFIX)` for the base environment instead of the child environment, where we actually see this equality. This pull request fixes that and updates the tests correspondingly. ## Test Plan I updated the existing tests with the new behavior. Let me know if you want more tests. Note: It shouldn't be necessary to test for the case where we have `conda/envs/base`, since one should not be able to create such an environment (one with the name of `CONDA_DEFAULT_ENV`). --------- Co-authored-by: Aria Desires <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a port of the logic in astral-sh/uv#7691
The basic idea is we use CONDA_DEFAULT_ENV as a signal for whether CONDA_PREFIX is just the ambient system conda install, or the user has explicitly activated a custom one. If the former, then the conda is treated like a system install (having lowest priority). If the latter, the conda is treated like an activated venv (having priority over everything but an Actual activated venv).
Fixes astral-sh/ty#611