Skip to content
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

Open
wants to merge 140 commits into
base: main
Choose a base branch
from
Open

Refactor for libmamba v2 (WIP) #457

wants to merge 140 commits into from

Conversation

jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Mar 26, 2024

Description

Adds compatibility for libmamba v2, which enables a few refactors (see below). Supersedes conda/conda#414.

Progress:

  • Refactor index helpers
  • Refactor repoquery
  • Refactor solver
  • Refactor conflict reports
  • Pass all tests
  • Polish and clean up

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?

Summary of changes

  • Deprecate CONDA_LIBMAMBA_SOLVER_NO_CHANNELS_FROM_INSTALLED. No longer needed with v2. Closes pip-integration: --dry-run still installs with pip conda#411
  • Added typing for all methods and functions
  • Added time_recorder decorator for metadata collection and solving loop
  • Removed state.BaseIndexHelper. We just have our index.IndexHelper now
  • The libmamba v1 pool (collection of repos, where a repo represents a loaded repodata.json) is now a database of RepoInfo objects.
  • Do use current_repodata.json if explicitly set in CLI
  • Move to Ruff for pre-commit linting & formatting
  • Logging from libsolv has a HUGE overhead now because it goes from C to C++ to Python to logging to stdout instead of C -> stdout. To enable libsolv verbosity, an extra environment variable CONDA_LIBMAMBA_SOLVER_DEBUG_LIBSOLV is required.
  • allow_uninstall was previously set to false (only true for conda 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.
  • Other changes in the test suite discussed in Amend test suite for compatibility with libmamba v2 conda#13784

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Mar 26, 2024
@jaimergp
Copy link
Contributor Author

pre-commit.ci autofix

@jaimergp jaimergp added the build::review trigger a build for this PR label Jul 19, 2024
@conda-bot conda-bot removed the build::review trigger a build for this PR label Jul 19, 2024
@conda-bot

This comment was marked as outdated.

@jaimergp jaimergp marked this pull request as ready for review July 19, 2024 08:34
@jaimergp
Copy link
Contributor Author

Since the canary workflow has a token issue for the uploads, I built it locally.

conda install -c jaimergp/label/conda-libmamba-solver_dev -c conda-forge/label/mamba_dev -c conda-forge conda-libmamba-solver

@conda-bot
Copy link
Contributor

conda-bot commented Jul 19, 2024

Review build status

The workflow for building and uploading a canary release for conda-libmamba-solver with the label conda-libmamba-solver-pr-457 in channel conda-canary was started by @jaimergp!

Once it's done, use this command to try it out in a new conda environment:

conda install -c conda-canary/label/conda-libmamba-solver-pr-457 conda-libmamba-solver

@jaimergp
Copy link
Contributor Author

jaimergp commented Jul 30, 2024

Those macOS crashes can be reproduced with:

>>> from libmambapy.specs import MatchSpec
>>> MatchSpec.parse("small-executable 1.0.0 0")
[1]    74412 trace trap  python

Apparently this is caused by libcxx 18. PR in mamba: mamba-org/mamba#3372

@jaimergp jaimergp added the build::review trigger a build for this PR label Aug 20, 2024
@conda-bot conda-bot removed the build::review trigger a build for this PR label Aug 20, 2024
@jaimergp
Copy link
Contributor Author

All green CI with the v2rc2, no code changes required.

Copy link
Member

@jezdez jezdez left a 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!

.github/workflows/builds-review.yaml Show resolved Hide resolved
repository: conda/conda # CONDA-LIBMAMBA-SOLVER CHANGE
# repository: conda/conda # CONDA-LIBMAMBA-SOLVER CHANGE
repository: jaimergp/conda # TEMPORARY
ref: libmamba-v2-fixes # TEMPORARY
Copy link
Member

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?

Copy link
Contributor Author

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 😬

Copy link
Member

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.

conda_libmamba_solver/index.py Show resolved Hide resolved
conda_libmamba_solver/index.py Outdated Show resolved Hide resolved
conda_libmamba_solver/mamba_utils.py Show resolved Hide resolved
conda_libmamba_solver/solver.py Show resolved Hide resolved
conda_libmamba_solver/solver.py Show resolved Hide resolved
conda_libmamba_solver/solver.py Show resolved Hide resolved
Comment on lines +699 to +700
# libmamba v2 has this message for package not found errors
# we need to double check with the explained problem
Copy link
Member

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

Copy link
Contributor Author

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 😬

docs/user-guide/performance.md Show resolved Hide resolved
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: 🆕 New
3 participants