Skip to content

Fix add_pip_as_python_dependency not respected with sharded repodata#929

Merged
dholth merged 23 commits intomainfrom
fix-pip-sharded
May 1, 2026
Merged

Fix add_pip_as_python_dependency not respected with sharded repodata#929
dholth merged 23 commits intomainfrom
fix-pip-sharded

Conversation

@danyeaw
Copy link
Copy Markdown
Member

@danyeaw danyeaw commented Apr 30, 2026

Description

Fixes #918 by passing in add_pip_as_python_dependency to the db. I also cleaned up a surrounding test to use pytest-mocker instead of unittest.mocker.

Assisted by Claude Sonnet 4.6.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@github-project-automation github-project-automation Bot moved this to 🆕 New in 🔎 Review Apr 30, 2026
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Apr 30, 2026
@danyeaw danyeaw marked this pull request as ready for review April 30, 2026 14:14
@danyeaw danyeaw requested a review from a team as a code owner April 30, 2026 14:14
Copy link
Copy Markdown
Contributor

@dholth dholth left a comment

Choose a reason for hiding this comment

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

This is great; I thought we would have to override shards "list of dependencies for package" but apparently we just have to send a boolean to libmamba.

Comment thread tests/test_index.py Outdated
assert python_records

pip_in_depends = any(
dep == "pip" or dep.startswith("pip ") for dep in (python_records[0].depends or [])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
dep == "pip" or dep.startswith("pip ") for dep in (python_records[0].depends or [])
dep.startswith("pip") for dep in (python_records[0].depends or [])

Would be a looser check.

Can it be one or the other? I would kindof expect that the pip dependency is always an exact string?

We could use the sharded def spec_to_package_name(spec: str) -> str: (a simple MatchSpec call) to really

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hi @dholth, I like the idea to use spec_to_package_name - nice one! I updated the test to use this.

@danyeaw
Copy link
Copy Markdown
Member Author

danyeaw commented Apr 30, 2026

It looks like this will need to come in after #924, since we are getting some CI failures.

dholth and others added 11 commits May 1, 2026 10:31
Use environment variable and reset_context() instead of
monkeypatch.setattr.()

Reverts change from commit abb7bed that was intended to fix an
AssertionError on Windows but introduced a regression in CI.
…cy_sharded

This assertion will help diagnose CI failures by verifying that the
context.add_pip_as_python_dependency flag is correctly set after calling
reset_context(), ensuring the monkeypatch environment variable is properly
applied.
Verify that the python package was loaded with the correct name and subdir.
This will help diagnose if there are any issues with package loading or
platform matching in the mamba database.
Instead of special-case handling after shard_mentioned_packages(),
use the existing 'extra' parameter to include pip when processing
python packages and add_pip_as_python_dependency is enabled.

This simplifies the code in both BFS and pipelined traversal algorithms,
ensuring both produce identical results.
@dholth dholth force-pushed the fix-pip-sharded branch from 3706a6c to 533d5a9 Compare May 1, 2026 17:22
@danyeaw
Copy link
Copy Markdown
Member Author

danyeaw commented May 1, 2026

pre-commit.ci autofix

@danyeaw
Copy link
Copy Markdown
Member Author

danyeaw commented May 1, 2026

pre-commit.ci autofix

Copy link
Copy Markdown
Member Author

@danyeaw danyeaw left a comment

Choose a reason for hiding this comment

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

I'm good with the changes @dholth pushed (ty!). We need to drop the tests.yml changes prior to merging.

Comment thread tests/test_index.py
@dholth
Copy link
Copy Markdown
Contributor

dholth commented May 1, 2026

It looks like libmamba's "add pip as Python dependency" code happens in the parse-json path, which we sidestep in our sharded repodata implementation.
Instead, we've chosen to add "pip" as a Python dependency in two places

  1. When traversing shards, we simply ensure that we include the "pip" shard(s) in our subset but we don't mutate the dependency list, keeping intact the data we received from the channel.
  2. When converting individual package records to libmamba objects, we append "pip" to the list of dependencies sent to the solver. We copy logic from conda's SubdirData and only append "pip" if the Python version starts with "2." or "3.". Why does it make that check? Who knows, but our goal is to behave the same way as conda.

Copy link
Copy Markdown
Contributor

@dholth dholth left a comment

Choose a reason for hiding this comment

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

Why not.

@github-project-automation github-project-automation Bot moved this from 🆕 New to ✅ Approved in 🔎 Review May 1, 2026
@dholth dholth merged commit 776b0c1 into main May 1, 2026
141 of 147 checks passed
@dholth dholth deleted the fix-pip-sharded branch May 1, 2026 22:41
@github-project-automation github-project-automation Bot moved this from ✅ Approved to 🏁 Done in 🔎 Review May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed [bot] added once the contributor has signed the CLA

Projects

Status: 🏁 Done

Development

Successfully merging this pull request may close these issues.

add_pip_as_python_dependency not honored when sharded repodata is enabled

3 participants