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

More explicit logic for CONDA_LIBMAMBA_SOLVER_MAX_ATTEMPTS #394

Merged
merged 6 commits into from
Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 35 additions & 5 deletions conda_libmamba_solver/solver.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
from conda.core.prefix_data import PrefixData
from conda.core.solve import Solver
from conda.exceptions import (
CondaValueError,
InvalidMatchSpec,
InvalidSpec,
PackagesNotFoundError,
Expand All @@ -62,6 +63,7 @@ class LibMambaSolver(Solver):
Cleaner implementation using the ``state`` module helpers.
"""

MAX_SOLVER_ATTEMPTS_CAP = 10
_uses_ssc = False

def __init__(
Expand Down Expand Up @@ -256,18 +258,46 @@ def _spinner_msg_solving(self):
return "Getting pinned dependencies"
return "Solving environment"

def _max_attempts(self, in_state: SolverInputState, default: int = 1):
from_env_var = os.environ.get("CONDA_LIBMAMBA_SOLVER_MAX_ATTEMPTS")
installed_count = len(in_state.installed)
if from_env_var:
try:
max_attempts_from_env = int(from_env_var)
except ValueError:
raise CondaValueError(
f"CONDA_LIBMAMBA_SOLVER_MAX_ATTEMPTS='{from_env_var}'. Must be int."
)
if max_attempts_from_env < 1:
raise CondaValueError(
f"CONDA_LIBMAMBA_SOLVER_MAX_ATTEMPTS='{max_attempts_from_env}'. Must be >=1."
)
elif max_attempts_from_env > installed_count:
log.warning(
"CONDA_LIBMAMBA_SOLVER_MAX_ATTEMPTS='%s' is higher than the number of "
"installed packages (%s). Using that one instead.",
max_attempts_from_env,
installed_count,
)
return installed_count
else:
return max_attempts_from_env
elif in_state.update_modifier.FREEZE_INSTALLED:
# this the default, but can be overriden with --update-specs
# we cap at MAX_SOLVER_ATTEMPTS_CAP attempts to avoid things
# getting too slow in large environments
return min(self.MAX_SOLVER_ATTEMPTS_CAP, installed_count)
else:
return default

def _solving_loop(
self,
in_state: SolverInputState,
out_state: SolverOutputState,
index: LibMambaIndexHelper,
):
solved = False
max_attempts = max(
2,
int(os.environ.get("CONDA_LIBMAMBA_SOLVER_MAX_ATTEMPTS", len(in_state.installed))) + 1,
)
for attempt in range(1, max_attempts):
for attempt in range(1, self._max_attempts(in_state) + 1):
Copy link
Member

Choose a reason for hiding this comment

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

Would it be useful to prepopulate the max attempts at the start of the solve as a instance attribute, instead of inline, so it's easier to debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this might break some tests. It shouldn't, but by saving it as instance attribute we are assuming we are going to have the same number of attempts for every call to solve_for_*() (for the lifetime of the same instance). In some instances, that might be tied to len(in_state.installed).

So I'd rather keep it like it is because it's technically more correct and does not assume that "one instantiation, one solve".

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, thank you!

log.debug("Starting solver attempt %s", attempt)
try:
solved = self._solve_attempt(in_state, out_state, index, attempt=attempt)
Expand Down
19 changes: 19 additions & 0 deletions news/394-max-attempts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
### Enhancements

* Add some boundary checks to `CONDA_LIBMAMBA_SOLVER_MAX_ATTEMPTS`. (#394)

### Bug fixes

* <news item>

### Deprecations

* <news item>

### Docs

* <news item>

### Other

* <news item>
Loading