Skip to content

Comments

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

Merged
Gankra merged 3 commits intoastral-sh:mainfrom
Danielkonge:conda-base
Oct 3, 2025
Merged

[ty] improve base conda distinction from child conda#20675
Gankra merged 3 commits intoastral-sh:mainfrom
Danielkonge:conda-base

Conversation

@Danielkonge
Copy link
Contributor

@Danielkonge Danielkonge commented Oct 1, 2025

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

Danielkonge commented Oct 2, 2025

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

Danielkonge commented Oct 2, 2025

@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