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

Remove old logic in SolverOutputState.prepare_specs() and apply enabled cleanups #381

Merged
merged 27 commits into from
Jan 3, 2024

Conversation

jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Nov 14, 2023

Description

In conda-libmamba-solver 23.9.0 we changed how we build the tasks for the libmambapy.Solver.add_jobs() method (#270). This allowed us to remove state.SolverOutputState.prepare_specs(), which tried to agnostically mimic classic's solver logic to add specs to pycosat, since we were not using it.

Since all the task state is now encoded in the tasks dictionary we iterate over before calling Solver.add_jobs(), we can also remove all the TrackedMap business (dictionaries whose operations had logged 'reasons'). Less code to maintain, less RAM used, everyone's happy.

Finally, since we have to iterate over some list of MatchSpec names, in some order (in #378 we have decided to do it alphabetically, but that's totally arbitrary), I took the opportunity to provide a SolverOutputState.specs property that iterates over all possible spec sources in a certain order that makes certain sense. Within each group, strictest specs go first (because we assume the user is putting extra effort in being explicit, so it must be important).

Spec order in each task is important for libsolv. Apparently first specs are considered earlier in the backtracking.

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?

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Nov 14, 2023
@jezdez
Copy link
Member

jezdez commented Nov 15, 2023

Woah!

@jaimergp
Copy link
Contributor Author

Whatever is going on with #317, this PR reproduces it reliably on Windows in the last few runs 🤷

@jaimergp jaimergp closed this Nov 27, 2023
@jaimergp jaimergp reopened this Nov 27, 2023
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.

A few questions mostly, I may need a session of elaboration about this refactoring, my brain keeps not forgetting the start of the PR when I'm in the middle

conda_libmamba_solver/solver.py Show resolved Hide resolved
conda_libmamba_solver/solver.py Outdated 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
conda_libmamba_solver/solver.py Show resolved Hide resolved
conda_libmamba_solver/state.py Show resolved Hide resolved
dev/linux/upstream_integration.sh Outdated Show resolved Hide resolved
dev/linux/upstream_integration.sh Show resolved Hide resolved
dev/linux/upstream_integration.sh Show resolved Hide resolved
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.

Just some minor questions and naming inconsistencies.

conda_libmamba_solver/index.py Show resolved Hide resolved
conda_libmamba_solver/state.py Show resolved Hide resolved
tests/test_state.py Outdated Show resolved Hide resolved
@jezdez jezdez changed the base branch from main to 23.12.x December 8, 2023 10:44
@jaimergp
Copy link
Contributor Author

There's something off with test_install_update_deps_only_deps_flags. I need to look into it.

@jezdez jezdez changed the base branch from 23.12.x to main December 19, 2023 09:41
@jezdez
Copy link
Member

jezdez commented Dec 19, 2023

Changed the base back to main, now that 23.12.x is out.

@jaimergp jaimergp closed this Jan 2, 2024
@jaimergp jaimergp reopened this Jan 2, 2024
@jaimergp
Copy link
Contributor Author

jaimergp commented Jan 2, 2024

There's something off with test_install_update_deps_only_deps_flags. I need to look into it.

It fixed itself :D

@jaimergp jaimergp marked this pull request as ready for review January 2, 2024 14:42
@jezdez jezdez merged commit 4652133 into main Jan 3, 2024
132 of 140 checks passed
@jezdez jezdez deleted the cleanup branch January 3, 2024 23:11
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
Archived in project
Development

Successfully merging this pull request may close these issues.

Hard crash for downgrade request (segfault or ... python: free(): invalid next size ...)
3 participants