Skip to content

Commit

Permalink
Do not allow pin overrides via requested specs in the CLI (#289)
Browse files Browse the repository at this point in the history 
* add compatible_matchspecs (MatchSpec.match is for PackageRecords only)

* more useful exception

* add test

* add news

* pre-commit

* pre-commit

* fix boolean logic

* simplify

* commit progress so far

* remove test

* add new exception for request and pinned errors

* do not allow pinned and requested specs

* cleanup

* pre-commit

* add tests

* pre-commit

* add docs

* ensure pins are in SolverOutputState.specs even if not explicit

* format exception

* raise earlier, no index needed

* fix state tests

* pre-commit

* relax test constraints

* better error messages in unsolvable pins

* fix test

* pre-commit

* override channels

* amend news

* fix test

* override channels here too

* Apply suggestions from code review

Co-authored-by: Travis Hathaway <travis.j.hathaway@gmail.com>

* Apply suggestions from code review

* Allow only CLI specs as long as compatible with pins (#294)

* implement some compatibility checks

* clean up a bit and add more tests

* pre-commit

* be explicit about channels

* simplify error logic for installed pins

* pre-commit

---------

Co-authored-by: Travis Hathaway <travis.j.hathaway@gmail.com>
  • Loading branch information
@jaimergp @travishathaway
jaimergp and travishathaway authored Sep 26, 2023
1 parent c82b9a0 commit a07d994
Show file tree
Hide file tree
Showing 9 changed files with 345 additions and 96 deletions.
33 changes: 18 additions & 15 deletions conda_libmamba_solver/solver.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -463,10 +463,13 @@ def _specs_to_tasks_add(self, in_state: SolverInputState, out_state: SolverOutpu
python_version_might_change = not to_be_installed_python.match(installed_python)

# Add specs to install
for name, spec in out_state.specs.items():
# SolverOutputState.specs has been populated at initialization with what the classic
# logic considers should be the target version for each package in the environment
# and requested changes. We are _not_ following those targets here, but we do iterate
# over the list to decide what to do with each package.
for name, _classic_logic_spec in out_state.specs.items():
if name.startswith("__"):
continue # ignore virtual packages
spec: MatchSpec = self._check_spec_compat(spec)
installed: PackageRecord = in_state.installed.get(name)
if installed:
installed_spec_str = self._spec_to_str(installed.to_match_spec())
Expand All @@ -481,21 +484,22 @@ def _specs_to_tasks_add(self, in_state: SolverInputState, out_state: SolverOutpu
tasks[("USERINSTALLED", api.SOLVER_USERINSTALLED)].append(installed_spec_str)

# These specs are explicit in some sort of way
if pinned:
if pinned and not pinned.is_name_only_spec:
# these are the EXPLICIT pins; conda also uses implicit pinning to
# constrain updates too but those can be overridden in case of conflicts.
if pinned.is_name_only_spec:
# pins need to constrain in some way, otherwide is undefined behaviour
pass
elif requested and not requested.match(pinned):
# We don't pin; requested and pinned are different and incompatible,
# requested wins and we let that happen in the next block
pass
else:
tasks[("ADD_PIN", api.SOLVER_NOOP)].append(self._spec_to_str(pinned))

# name-only pins are treated as locks when installed, see below
tasks[("ADD_PIN", api.SOLVER_NOOP)].append(self._spec_to_str(pinned))
# in libmamba, pins and installs are compatible tasks (pin only constrains,
# does not 'request' a package). In classic, pins were actually targeted installs
# so they were exclusive
if requested:
spec_str = self._spec_to_str(requested)
if requested.is_name_only_spec and pinned and not pinned.is_name_only_spec:
# for name-only specs, this is a no-op; we already added the pin above
# but we will constrain it again in the install task to have better
# error messages if not solvable
spec_str = self._spec_to_str(pinned)
else:
spec_str = self._spec_to_str(requested)
if installed:
tasks[("UPDATE", api.SOLVER_UPDATE)].append(spec_str)
tasks[("ALLOW_UNINSTALL", api.SOLVER_ALLOWUNINSTALL)].append(name)
Expand Down Expand Up @@ -776,7 +780,6 @@ def _explain_with_pins(self, message, pins):
pin_message = "Pins seem to be involved in the conflict. Currently pinned specs:\n"
for pin_name, spec in pins.items():
pin_message += f" - {spec} (labeled as '{pin_name}')\n"
pin_message += "\nIf python is involved, try adding it explicitly to the command-line."
return f"{message}\n\n{pin_message}"
return message

Expand Down
151 changes: 81 additions & 70 deletions conda_libmamba_solver/state.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@
# TODO: This module could be part of conda-core once if we refactor the classic logic

import logging
from itertools import chain
from os import PathLike
from types import MappingProxyType
from typing import Iterable, Mapping, Optional, Type, Union
Expand All @@ -87,6 +86,7 @@
from conda.models.records import PackageRecord

from .models import EnumAsBools, TrackedMap
from .utils import compatible_specs

log = logging.getLogger(f"conda.{__name__}")

Expand Down Expand Up @@ -212,18 +212,6 @@ def __init__(
# special cases
self._do_not_remove = {p: MatchSpec(p) for p in self._DO_NOT_REMOVE_NAMES}

self._check_state()

def _check_state(self):
"""
Run some consistency checks to ensure configuration is solid.
"""
# Ensure configured pins match installed builds
for name, pin_spec in self.pinned.items():
installed = self.installed.get(name)
if installed and not pin_spec.match(installed):
raise SpecsConfigurationConflictError([installed], [pin_spec], self.prefix)

def _default_to_context_if_null(self, name, value, context=context):
"Obtain default value from the context if value is set to NULL; otherwise leave as is"
return getattr(context, name) if value is NULL else self._ENUM_STR_MAP.get(value, value)
Expand Down Expand Up @@ -622,14 +610,13 @@ def virtual_specs(self):

def prepare_specs(self, index: IndexHelper) -> Mapping[str, MatchSpec]:
"""
Main method to populate the ``specs`` mapping. ``index`` is needed only
in some cases, and only to call its method ``.explicit_pool()``.
Main method to populate the ``specs`` mapping.
"""
if self.solver_input_state.is_removing:
self._prepare_for_remove()
else:
self._prepare_for_add(index)
self._prepare_for_solve()
self._prepare_for_solve(index)
return self.specs

def _prepare_for_add(self, index: IndexHelper):
Expand Down Expand Up @@ -711,15 +698,23 @@ def _prepare_for_add(self, index: IndexHelper):
# - requested by the user (if request and pin conflict, request takes precedence)
# - a dependency of something requested by the user
pin_overrides = set()
if sis.pinned:
explicit_pool = set(index.explicit_pool(sis.requested.values()))
# The block using this object below has been deactivated.
# so we don't build this (potentially expensive) set anymore
# if sis.pinned:
# explicit_pool = set(index.explicit_pool(sis.requested.values()))
for name, spec in sis.pinned.items():
pin = MatchSpec(spec, optional=False)
requested = name in sis.requested
if name in sis.installed and not requested:
self.specs.set(name, pin, reason="Pinned, installed and not requested")
elif requested:
if sis.requested[name].match(spec):
# THIS BLOCK WOULD NEVER RUN
# classic solver would check compatibility between pinned and requested
# and let the user override pins in the CLI. libmamba doesn't allow
# the user to override pins. We will have raised an exception earlier
# We will keep this code here for reference
if True: # compatible_specs(index, sis.requested[name], spec):
# assume compatible, we will raise later otherwise
reason = (
"Pinned, installed and requested; constraining request "
"as pin because they are compatible"
Expand All @@ -732,26 +727,35 @@ def _prepare_for_add(self, index: IndexHelper):
"are conflicting, so adding user request due to higher precedence"
)
self.specs.set(name, sis.requested[name], reason=reason)
elif name in explicit_pool:
# TODO: This might be introducing additional specs into the list if the pin
# matches a dependency of a request, but that dependency only appears in _some_
# of the request variants. For example, package A=2 depends on B, but package
# A=3 no longer depends on B. B will be part of A's explicit pool because it
# "could" be a dependency. If B happens to be pinned but A=3 ends up being the
# one chosen by the solver, then B would be included in the solution when it
# shouldn't. It's a corner case but it can happen so we might need to further
# restrict the explicit_pool to see. The original logic in the classic solver
# checked: `if explicit_pool[s.name] & ssc.r._get_package_pool([s]).get(s.name,
# set()):`
# always assume the pin will be needed
# elif name in explicit_pool:
# THIS BLOCK HAS BEEN DEACTIVATED
# the explicit pool is potentially expensive and we are not using it.
# leaving this here for reference. It's supposed to check whether the pin
# was going to be part of the environment because it shows up in the dependency
# tree of the explicitly requested specs.
# ---
# TODO: This might be introducing additional specs into the list if the pin
# matches a dependency of a request, but that dependency only appears in _some_
# of the request variants. For example, package A=2 depends on B, but package
# A=3 no longer depends on B. B will be part of A's explicit pool because it
# "could" be a dependency. If B happens to be pinned but A=3 ends up being the
# one chosen by the solver, then B would be included in the solution when it
# shouldn't. It's a corner case but it can happen so we might need to further
# restrict the explicit_pool to see. The original logic in the classic solver
# checked: `if explicit_pool[s.name] & ssc.r._get_package_pool([s]).get(s.name,
# set()):`
else: # always add the pin for libmamba to consider it
self.specs.set(
name,
pin,
reason="Pin matches one of the potential dependencies of user requests",
)
else:
log.warn(
"pinned spec %s conflicts with explicit specs. Overriding pinned spec.", spec
)
# In classic, this would notify the pin was being overridden by a request
# else:
# log.warn(
# "pinned spec %s conflicts with explicit specs. Overriding pinned spec.", spec
# )

# ## Update modifiers ###

Expand Down Expand Up @@ -941,7 +945,7 @@ def _prepare_for_remove(self):
# This logic is simpler than when we are installing packages
self.specs.update(self.solver_input_state.requested, reason="Adding user-requested specs")

def _prepare_for_solve(self):
def _prepare_for_solve(self, index):
"""
Last part of the logic, common to addition and removal of packages. Originally,
the legacy logic will also minimize the conflicts here by doing a pre-solve
Expand All @@ -953,23 +957,30 @@ def _prepare_for_solve(self):
# ## Inconsistency analysis ###
# here we would call conda.core.solve.classic.Solver._find_inconsistent_packages()

# ## Check conflicts are only present in .specs
conflicting_and_pinned = [
str(spec)
for name, spec in self.conflicts.items()
if name in self.solver_input_state.pinned
]
if conflicting_and_pinned:
requested = [
str(spec)
for spec in chain(self.specs, self.solver_input_state.requested)
if spec not in conflicting_and_pinned
]
raise SpecsConfigurationConflictError(
requested_specs=requested,
pinned_specs=conflicting_and_pinned,
prefix=self.solver_input_state.prefix,
)
# ## Check pin and requested are compatible
sis = self.solver_input_state
requested_and_pinned = set(sis.requested).intersection(sis.pinned)
for name in requested_and_pinned:
requested = sis.requested[name]
pin = sis.pinned[name]
installed = sis.installed.get(name)
if (
# name-only pins lock to installed; requested spec must match it
(pin.is_name_only_spec and installed and not requested.match(installed))
# otherwise, the pin needs to be compatible with the requested spec
or not compatible_specs(index, (requested, pin))
):
pinned_specs = [
(sis.installed.get(name, pin) if pin.is_name_only_spec else pin)
for name, pin in sorted(sis.pinned.items())
]
exc = SpecsConfigurationConflictError(
requested_specs=sorted(sis.requested.values(), key=lambda x: x.name),
pinned_specs=pinned_specs,
prefix=sis.prefix,
)
exc.allow_retry = False
raise exc

# ## Conflict minimization ###
# here conda.core.solve.classic.Solver._run_sat() enters a `while conflicting_specs` loop
Expand All @@ -981,18 +992,26 @@ def _prepare_for_solve(self):

def early_exit(self):
"""
Operations that do not need a solver at all and might result in returning early
are collected here.
Operations that do not need a solver and might result in returning
early are collected here.
"""
sis = self.solver_input_state

if sis.is_removing and sis.force_remove:
for name, spec in sis.requested.items():
for record in sis.installed.values():
if spec.match(record):
self.records.pop(name)
break
return self.current_solution
if sis.is_removing:
not_installed = [
spec for name, spec in sis.requested.items() if name not in sis.installed
]
if not_installed:
exc = PackagesNotFoundError(not_installed)
exc.allow_retry = False
raise exc

if sis.force_remove:
for name, spec in sis.requested.items():
for record in sis.installed.values():
if spec.match(record):
self.records.pop(name)
break
return self.current_solution

if sis.update_modifier.SPECS_SATISFIED_SKIP_SOLVE and not sis.is_removing:
for name, spec in sis.requested.items():
Expand All @@ -1004,14 +1023,6 @@ def early_exit(self):
# to the map of installed packages)
return self.current_solution

# Check we are not trying to remove things that are not installed
if sis.is_removing:
not_installed = [
spec for name, spec in sis.requested.items() if name not in sis.installed
]
if not_installed:
raise PackagesNotFoundError(not_installed)

def post_solve(self, solver: Type["Solver"]):
"""
These tasks are performed _after_ the solver has done its work. It essentially
Expand Down
36 changes: 36 additions & 0 deletions conda_libmamba_solver/utils.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from conda.common.compat import on_win
from conda.common.path import url_to_path
from conda.common.url import urlparse
from conda.exceptions import PackagesNotFoundError
from conda.gateways.connection import session as gateway_session

log = getLogger(f"conda.{__name__}")
Expand Down Expand Up @@ -55,3 +56,38 @@ def is_channel_available(channel_url) -> bool:
except Exception as exc:
log.debug("Failed to check if channel %s is available", channel_url, exc_info=exc)
return False


def compatible_specs(index, specs, raise_not_found=True):
"""
Assess whether the given specs are compatible with each other.
This is done by querying the index for each spec and taking the
intersection of the results. If the intersection is empty, the
specs are incompatible.
If raise_not_found is True, a PackagesNotFoundError will be raised
when one of the specs is not found. Otherwise, False will be returned
because the intersection will be empty.
"""
if not len(specs) >= 2:
raise ValueError("Must specify at least two specs")

matched = None
for spec in specs:
results = set(index.search(str(spec)))
if not results:
if raise_not_found:
exc = PackagesNotFoundError([spec], index._channels)
exc.allow_retry = False
raise exc
return False
if matched is None:
# First spec, just set matched to the results
matched = results
continue
# Take the intersection of the results
matched &= results
if not matched:
return False

return bool(matched)
2 changes: 2 additions & 0 deletions dev/collect_upstream_conda_tests/collect_upstream_conda_tests.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@
"test_offline_with_empty_index_cache",
# Adjusted in tests/test_modified_upstream.py
"test_install_features",
# libmamba departs from this behavior in the classic logic
# see https://github.com/conda/conda-libmamba-solver/pull/289
"test_pinned_override_with_explicit_spec",
# TODO: https://github.com/conda/conda-libmamba-solver/issues/141
"test_conda_pip_interop_conda_editable_package",
Expand Down
Loading

0 comments on commit a07d994

Please sign in to comment.