-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
Refactor for libmamba v2 (WIP) #457
base: main
Are you sure you want to change the base?
Conversation
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
…el for CLI names, fix subdir URL, readd ChannelInfo dataclass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments for clarification, but this looks really really good!
repository: conda/conda # CONDA-LIBMAMBA-SOLVER CHANGE | ||
# repository: conda/conda # CONDA-LIBMAMBA-SOLVER CHANGE | ||
repository: jaimergp/conda # TEMPORARY | ||
ref: libmamba-v2-fixes # TEMPORARY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's needed to make this use main?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge conda/conda#13784. But it has this weird bootstrapping problem because those adjusted tests need this PR to be merged to pass 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instant classic! I mean, the real solution would be a third test running repo, right? In the meantime, we can clarify this in the upstream repo and mention this PR to make an exception IMO. Just say the word and I leave a statement about this in the conda PR so we can merge it.
# libmamba v2 has this message for package not found errors | ||
# we need to double check with the explained problem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be excellent to clean up somehow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we can't get the "explained problems" in a structured way directly. We could get the graph (Unsolvable.problems_graph()
) but then we need to redo the logic to obtain the actual conflicts, and that involves several Tree*
objects only available in the C++ layer. So the simplest thing here is to parse line by line and trim the hierarchy indicators 😬
1ff8875
to
9397d7a
Compare
…lver into try-libmamba-2a3
…lver into try-libmamba-2a3
3de339b
to
f15ca13
Compare
Description
Adds compatibility for libmamba v2, which enables a few refactors (see below). Supersedes conda/conda#414.
Progress:
Checklist - did you ...
news
directory (using the template) for the next release's release notes?Summary of changes
CONDA_LIBMAMBA_SOLVER_NO_CHANNELS_FROM_INSTALLED
. No longer needed with v2. Closes pip-integration: --dry-run still installs with pip conda#411time_recorder
decorator for metadata collection and solving loopstate.BaseIndexHelper
. We just have ourindex.IndexHelper
nowpool
(collection ofrepos
, where arepo
represents a loadedrepodata.json
) is now adatabase
ofRepoInfo
objects.current_repodata.json
if explicitly set in CLICONDA_LIBMAMBA_SOLVER_DEBUG_LIBSOLV
is required.allow_uninstall
was previously set tofalse
(onlytrue
forconda remove
), and we set it up for individual solver jobs involving updates or conflicts. With v2, we have individual control over what to "Keep" instead of drop. This required marking important updates as keepers instead. Otherwise they would be uninstalled.