Do not allow pin overrides via requested specs in the CLI #289

jaimergp · 2023-09-21T11:09:55Z

Description

Closes #286

The original intention of this PR was to address an issue in how we were checking whether pinned specs and explicitly requested specs were compatible or not. After careful investigation, we realized two things:

generalizing a robust solution would require querying the index in several occasions trying to anticipate something that the solver will anyway end up doing. This is RAM intensive and potentially more costly than the solve it self. It's not that bad.
This check is part of the "user-requested specs can override pinned specs" logic implemented in the classic solver. This behaviour has been addressed as surprising in issue Conda solver does not respect package specifications in the pinned file conda#9016.

As a result, in this PR we have decided to:

Prevent the user from overriding pinned specs with incompatible CLI specs. An early error will show up if that happens. Instead, we suggest being more explicit and adjust the pinned specs, or use --no-pin.
Formalize a "name-only pins lock if installed" policy. This adds support for pinned specs that are name-only (no version or build constraints). This was previously undefined behavior. For every name-only pinned spec:
- If the package is installed, we lock it. It won't be able to be change at all.
- If it's not installed, we don't do anything.
- If the user needs to modify that package, again, the pinned config needs to be modified explicitly or --no-pin can be used.
- The error message will render name-only pins as their installed counterparts if appropriate.

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_libmamba_solver/utils.py

wuillaum · 2023-09-21T15:37:11Z

I tested this PR using a similar approach to my Dockerfile in the related issue.

It appears that conflicts to a pinned file only raise a warning using the classic solver (honestly this is a pretty bad bug, and directly conflicts with the docs. I'm surprised the following issue has been around for so long given that conda used to behave correctly before conda/conda#9016)

regardless, there is still a difference in behavior between libmamba and classic, where libmamba won't even give a warning about pin conflicts. See below

FROM continuumio/miniconda3:23.5.2-0


WORKDIR /app

COPY requirements.txt .

# there isn't a published container for >=23.7, so this is my workaround
RUN conda upgrade conda

# I cloned and checked out your PR branch here
COPY conda-libmamba-solver conda-libmamba-solver

RUN /opt/conda/bin/python -m pip install /app/conda-libmamba-solver --no-deps --force-reinstall


RUN conda create -c conda-forge -n myenv --file requirements.txt --solver libmamba

COPY pinned .
RUN mv pinned /opt/conda/envs/myenv/conda-meta/pinned


# classic solver works as expected
RUN conda install -c conda-forge -n myenv --solver classic  --file requirements.txt

# this works now! wohoo :)
RUN conda install -c conda-forge -n myenv --solver libmamba --file requirements.txt


# here is where behavior differs
# the classic solver will give the following warning
# Solving environment: ...working... WARNING conda.core.solve:_add_specs(800): pinned spec pandas==1.2.5 conflicts with explicit specs.  Overriding pinned spec.

# but the libmamba solver doesn't give any warning (both proceed to update pandas despite the pin...)
RUN conda install -n myenv --solver libmamba pandas=1.3.5

jaimergp · 2023-09-21T16:35:42Z

I've been in a call with @costrouc debugging this, and the conclusion so far is that it makes little sense to anticipate conflicting pins as conda classic did (with the corresponding warning we can see in that output). We could follow the classic approach and query the index for packages that are compatible with each spec (pin and user-requested) and estimate the compatibility via their set intersection. If the intersection is not null, then both pins would be compatible. However this implies running some RAM intensive computations (JSON payloads are sent from C++ to python) and the solver will eventually error out if not compatible anyway.

So we thought, let's both pin and request the install, and the solver error will report. That sounds good, BUT conda has this notion of "user can override pins if explicitly requested in the CLI", which makes the above statement not as useful. So the current code as of commit af11b5d will always give more precedence to the user requested spec.

This will probably fail a couple tests here and there. We'll see and try to decide what to do next.

conda_libmamba_solver/solver.py

jezdez · 2023-09-22T09:48:34Z

Let's make a call in favor of user experience and note the difference in behavior (and respecting the pining via file first) in the conda-libmamba-solver release notes.

conda_libmamba_solver/state.py

conda_libmamba_solver/solver.py

jaimergp · 2023-09-25T11:24:30Z

conda_libmamba_solver/solver.py

+                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)


Note this is purely for UX purposes. The solver doesn't care if we do either of:

A:

install("python") pin("python=3.7")

B:

install("python=3.7") pin("python=3.7")

However, B will give (as of libmamba 1.5.x) a clearer error message if the request happens to be unsolvable.

jaimergp · 2023-09-25T11:26:17Z

conda_libmamba_solver/state.py

+        # Check we are not trying to remove things that are not installed
+        if self.is_removing:
+            not_installed = [
+                spec for name, spec in self.requested.items() if name not in self.installed
+            ]
+            if not_installed:
+                exc = PackagesNotFoundError(not_installed)
+                exc.allow_retry = False
+                raise exc
+
+        # Check if requested and pins overlap
+        # NOTE: This is a difference with respect to classic logic. classic
+        # allows pin overrides in the CLI, but we don't.
+        constraining_requests = {
+            spec.name for spec in self.requested.values() if not spec.is_name_only_spec
+        }
+        constraining_pins = {spec.name for spec in self.pinned.values()}
+        requested_and_pinned = constraining_requests.intersection(constraining_pins)
+        if requested_and_pinned:
+            # libmamba has two types of pins:
+            # - Pins with constraints: limit which versions of a package can be installed
+            # - Name-only pins: lock the package as installed; has no effect if not installed
+            #   Below we render name-only pins as their installed version when appropriate.
+            pinned_specs = [
+                (self.installed.get(name, pin) if pin.is_name_only_spec else pin)
+                for name, pin in sorted(self.pinned.items())
+            ]
+            exc = RequestedAndPinnedError(
+                requested_specs=sorted(self.requested.values(), key=lambda x: x.name),
+                pinned_specs=pinned_specs,
+                prefix=self.prefix,
+            )
+            exc.allow_retry = False
+            raise exc


We moved these clauses here (from SolverOutputState) because at this point we haven't initialized the index (which means downloading repodata and parsing it, with the implied IO overheads). Since we don't need it to compute these errors, we can exit even earlier and happier users. Imagine waiting precious seconds downloading repodata only to realize you mistyped something in the CLI. Now it's mostly instantaneous.

docs/libmamba-vs-classic.md

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

conda_libmamba_solver/exceptions.py

costrouc

Looks good to me and I tested out locally via docker. Also a fan of having more straightforward rules for how pins/requests interact with each other.

costrouc · 2023-09-26T04:57:29Z

tests/test_solvers.py

@@ -236,3 +239,126 @@ def test_too_aggressive_update_to_conda_forge_packages():
        data_libmamba = json.loads(p_libmamba.stdout)
        assert len(data_classic["actions"]["LINK"]) < 15
        assert len(data_libmamba["actions"]["LINK"]) <= len(data_classic["actions"]["LINK"])
+
+
+@pytest.mark.skipif(context.subdir != "linux-64", reason="Linux-64 only")


Ran both of these tests locally with docker and with the latest of main on mamba and they passed.

* 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

jaimergp · 2023-09-26T17:42:33Z

conda_libmamba_solver/state.py

-        Run some consistency checks to ensure configuration is solid.
-        """
-        # Ensure configured pins match installed builds
-        for name, pin_spec in self.pinned.items():


An equivalent check is done now in SolverOutputState._prepare_for_solve()

jaimergp · 2023-09-26T21:54:38Z

Merging for now. We'll clean up in a follow up PR.

jaimergp added 3 commits September 21, 2023 13:08

add compatible_matchspecs (MatchSpec.match is for PackageRecords only)

2b2ce7f

more useful exception

10669b8

add test

b8ea80d

conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Sep 21, 2023

jaimergp added 2 commits September 21, 2023 13:11

add news

7658571

pre-commit

42610cd

jaimergp mentioned this pull request Sep 21, 2023

libmamba traceback when using a pin file on an existing env that used build_string #286

Closed

2 tasks

jaimergp added 3 commits September 21, 2023 13:23

pre-commit

53cdac9

fix boolean logic

4082cd5

simplify

fa2c789

jaimergp requested review from jezdez and costrouc September 21, 2023 14:42

jezdez reviewed Sep 21, 2023

View reviewed changes

conda_libmamba_solver/utils.py Outdated Show resolved Hide resolved

commit progress so far

af11b5d

jaimergp commented Sep 21, 2023

View reviewed changes

conda_libmamba_solver/solver.py Outdated Show resolved Hide resolved

costrouc self-assigned this Sep 22, 2023

jaimergp mentioned this pull request Sep 22, 2023

Conda solver does not respect package specifications in the pinned file conda/conda#9016

Closed

jaimergp added 5 commits September 24, 2023 13:10

remove test

9521532

add new exception for request and pinned errors

8f373e5

do not allow pinned and requested specs

2b3f329

cleanup

654eb93

pre-commit

50ae547

jaimergp changed the title ~~Check MatchSpec compatibility properly~~ Do not allow pin overrides via requested specs in the CLI Sep 24, 2023

jaimergp added 4 commits September 24, 2023 13:49

add tests

0eb6543

pre-commit

0330718

Merge branch 'main' into pin-ms-match

6c03cea

add docs

e7605ea

jaimergp added 2 commits September 24, 2023 19:24

amend news

bcbfbf0

fix test

0219a4f

jaimergp marked this pull request as ready for review September 25, 2023 08:35

jaimergp added this to the September 2023 release milestone Sep 25, 2023

jezdez approved these changes Sep 25, 2023

View reviewed changes

conda_libmamba_solver/state.py Show resolved Hide resolved

override channels here too

e2ad303

jaimergp commented Sep 25, 2023

View reviewed changes

conda_libmamba_solver/solver.py Show resolved Hide resolved

jaimergp commented Sep 25, 2023

View reviewed changes

travishathaway reviewed Sep 25, 2023

View reviewed changes