Skip to content

[ty] improve base conda distinction from child conda #20675

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 3, 2025
Merged

[ty] improve base conda distinction from child conda #20675

merged 3 commits into from
Oct 3, 2025

Conversation

Danielkonge
Copy link
Contributor

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).

@Danielkonge Danielkonge changed the title [ty] improve base conda distinction child conda [ty] improve base conda distinction from child conda Oct 1, 2025
@MichaReiser MichaReiser requested a review from Gankra October 2, 2025 06:26
@MichaReiser MichaReiser added the ty Multi-file analysis & type inference label Oct 2, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 2, 2025 •
edited
Loading

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 2, 2025 •
edited
Loading

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

@sharkdp sharkdp removed their request for review October 2, 2025 07:36
@Gankra
Copy link
Contributor

Gankra commented Oct 2, 2025

Hmm this implementation has seemingly been working fine for uv for quite a while?

https://github.com/astral-sh/uv/blob/8da9df3654f255a2f2e8e83c51257c0699ece9f7/crates/uv-python/src/virtualenv.rs#L79

Did I invert something in my implementation?

@Gankra
Copy link
Contributor

Gankra commented Oct 2, 2025 •
edited
Loading

Aha! uv just shipped this fix (and a bonus one) a couple weeks ago.

astral-sh/uv#15679
astral-sh/uv#15680

Gankra
Gankra approved these changes Oct 2, 2025
View reviewed changes
Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

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

Approving the basic idea, but some more comment/naming cleanups are in-order, ala the equivalent uv PRs. Would you be interested in doing those extra changes? If not I can.

@Danielkonge
Copy link
Contributor Author

Approving the basic idea, but some more comment/naming cleanups are in-order, ala the equivalent uv PRs. Would you be interested in doing those extra changes? If not I can.

I can try to add these changes later today.

@AlexWaygood AlexWaygood removed their request for review October 2, 2025 15:30
Danielkonge
Danielkonge commented Oct 2, 2025
View reviewed changes
crates/ty/docs/environment.md
Used to detect the path of an active Conda environment.
If both `VIRTUAL_ENV` and `CONDA_PREFIX` are present, `VIRTUAL_ENV` will be preferred.

### `_CONDA_ROOT`
Copy link
Contributor Author

@Danielkonge Danielkonge Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

uv writes CONDA_ROOT instead of _CONDA_ROOT, but the section is "Externally-defined variables", and the actual environment variable is with the _, so I wrote it like this.

@Danielkonge
Copy link
Contributor Author

@Gankra: I think I added what the mentioned uv PRs covered now. Let me know if you wanted more than this?

Also: I left it is in a separate commit for now, but should I just rebase?

Gankra
Gankra approved these changes Oct 3, 2025
View reviewed changes
Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

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

Rad, thanks so much!

@Gankra Gankra enabled auto-merge (squash) October 3, 2025 13:52
@Gankra Gankra merged commit f73ead1 into astral-sh:main Oct 3, 2025
38 checks passed
dcreager added a commit that referenced this pull request Oct 3, 2025
@dcreager
Merge remote-tracking branch 'origin/main' into dcreager/non-inherited
407f7bb 
* origin/main:
  [`flake8-bugbear`] Include certain guaranteed-mutable expressions: tuples, generators, and assignment expressions (`B006`) (#20024)
  [`flake8-comprehensions`] Clarify fix safety documentation (`C413`) (#20640)
  [ty] improve base conda distinction from child conda (#20675)
  [`ruff`] Extend FA102 with listed PEP 585-compatible APIs (#20659)
  [`ruff`] Handle argfile expansion errors gracefully (#20691)
  [`flynt`] Fix f-string quoting for mixed quote joiners (`FLY002`) (#20662)
  [ty] Fix file root matching for `/`
  [ruff,ty] Enable tracing's `log` feature
  [`flake8-annotations`] Fix return type annotations to handle shadowed builtin symbols (`ANN201`, `ANN202`, `ANN204`, `ANN205`, `ANN206`) (#20612)
  Bump 0.13.3 (#20685)
  Update benchmarking CI for cargo-codspeed v4 (#20686)
  [ty] Support single-starred argument for overload call (#20223)
  [ty] `~T` should never be assignable to `T` (#20606)
  [`pylint`] Clarify fix safety to include left-hand hashability (`PLR6201`) (#20518)
  [ty] No union with `Unknown` for module-global symbols (#20664)
  [`ty`] Reject renaming files to start with slash in Playground (#20666)
  [ty] Enums: allow multiple aliases to point to the same member (#20669)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Gankra Gankra Gankra approved these changes

@carljm carljm Awaiting requested review from carljm carljm is a code owner

@dcreager dcreager Awaiting requested review from dcreager dcreager is a code owner

@MichaReiser MichaReiser Awaiting requested review from MichaReiser MichaReiser is a code owner

Assignees

No one assigned

Labels

ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Danielkonge @Gankra @MichaReiser