conda-forge / conda-smithy

The tool for managing conda-forge feedstocks.
https://conda-forge.org/
BSD 3-Clause "New" or "Revised" License
152 stars 176 forks source link

Remote-ci-setup handling cannot handle constraints anymore (since conda-smithy 3.26) #1773

Closed h-vetinari closed 1 year ago

h-vetinari commented 1 year ago

It seems that https://github.com/conda-forge/conda-smithy/commit/4a5ac16fc18281dd8e5922c13b333d31b50dd73f broke handling of remote_ci_setup setups that use constraints. For example, for compiler-rt, we need to use:

remote_ci_setup:
  - conda-forge-ci-setup=3
  - py-lief<0.12

This will not rerender anymore with the newest conda-smithy (I tried various quoting and indentation patterns) -- from what I can tell this is due to https://github.com/conda-forge/conda-smithy/blob/8ffa0d8f73e69dbcfe19c97c92b05e77187cad6a/conda_smithy/configure_feedstock.py#L2039-L2041 using .name unconditionally.

This has effectively made compiler-rt un-rerenderable (except with manual fixes), because we cannot remove the py-lief pin, since https://github.com/conda/conda-build/issues/4787 was only fixed in conda-build 3.26, which we still cannot pull into our CI due to https://github.com/conda-forge/boa-feedstock/issues/75. For a sample CI failure without the pin, see https://github.com/conda-forge/compiler-rt-feedstock/pull/83.

CC @jaimergp

stacktrace ### Note the unbalanced quotes! ``` (builder) C:\Users\[...]\dev\conda-forge\compiler-rt-feedstock>conda smithy rerender INFO:conda_smithy.configure_feedstock:Downloading conda-forge-pinning-2023.09.26.03.46.41 INFO:conda_smithy.configure_feedstock:Extracting conda-forge-pinning to C:\Users\[...]\AppData\Local\Temp\tmpjn62ot_j Traceback (most recent call last): File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\version.py", line 43, in __call__ return cls._cache_[arg] KeyError: '<0.12"' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\version.py", line 43, in __call__ return cls._cache_[arg] KeyError: '0.12"' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "C:\Users\[...]\.conda\envs\builder\Scripts\conda-smithy-script.py", line 9, in sys.exit(main()) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda_smithy\cli.py", line 669, in main args.subcommand_func(args) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda_smithy\cli.py", line 485, in __call__ self._call(args, tmpdir) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda_smithy\cli.py", line 490, in _call configure_feedstock.main( File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda_smithy\configure_feedstock.py", line 2357, in main config = _load_forge_config(forge_dir, exclusive_config_file, forge_yml) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda_smithy\configure_feedstock.py", line 2039, in _load_forge_config config["remote_ci_setup_names"] = [ File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda_smithy\configure_feedstock.py", line 2040, in MatchSpec(pkg).name for pkg in config["remote_ci_setup"] File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\match_spec.py", line 55, in __call__ return super().__call__(**parsed) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\match_spec.py", line 179, in __init__ self._match_components = self._build_components(**kwargs) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\match_spec.py", line 414, in _build_components return frozendict(_make_component(key, value) for key, value in kwargs.items()) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\_vendor\frozendict\__init__.py", line 21, in __init__ self._dict = self.dict_cls(*args, **kwargs) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\match_spec.py", line 414, in return frozendict(_make_component(key, value) for key, value in kwargs.items()) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\match_spec.py", line 428, in _make_component matcher = _implementors[field_name](value) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\version.py", line 45, in __call__ val = cls._cache_[arg] = super().__call__(arg) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\version.py", line 516, in __init__ vspec_str, matcher, is_exact = self.get_matcher(vspec) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\version.py", line 570, in get_matcher self.matcher_vo = VersionOrder(vo_str) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\version.py", line 45, in __call__ val = cls._cache_[arg] = super().__call__(arg) File "C:\Users\[...]\.conda\envs\builder\lib\site-packages\conda\models\version.py", line 171, in __init__ raise InvalidVersionSpec(vstr, "invalid character(s)") conda.exceptions.InvalidVersionSpec: Invalid version '0.12"': invalid character(s) ```
jaimergp commented 1 year ago

Sorry about that. I added the 'names only' variant because conda update only allows name-only specs. mamba seems to accept constrained specs too, but I assumed it was an oversight, not an intentional feature. Is this on Windows only?

h-vetinari commented 1 year ago

Sorry about that. I added the 'names only' variant because conda update only allows name-only specs. mamba seems to accept constrained specs too, but I assumed it was an oversight, not an intentional feature.

Constraining infrastructure deps like lief seems to be the primary (and I believe motivating) use case of remote_ci_setup to me, so I don't think we can ignore it.

Is this on Windows only?

It was supported on all platforms, but in any case, the current issue (basically https://github.com/conda/conda-build/issues/4787) only occurs on osx, so that's where we'd mainly need it.

jaimergp commented 1 year ago

I'll put a PR later today with a fix.

jaimergp commented 1 year ago

In the meantime, I am curious: does conda_install_tool: conda + conda_solver: libmamba + conda_build_tool: conda-build in conda-forge.yml sort the issue?

h-vetinari commented 1 year ago

I have a PR for testing this, feel free to push there - I'm logging off for today 🙃

h-vetinari commented 1 year ago

If it manages to pull in conda-build 3.26, it will be fixed (one of the successful runs in that PR was in the brief period where we had boa builds that were compatible with 3.26, before they were marked broken)

h-vetinari commented 1 year ago

Well, unfortunately that didn't work completely. The rerender passes now, however, it strips the constraints on unix. Using the brand-new conda-smithy 3.26.3 on top of the (manually fixed) https://github.com/conda-forge/compiler-rt-feedstock/pull/85, I get:

diff --git a/.scripts/build_steps.sh b/.scripts/build_steps.sh
index d83ad42..6e1bcd8 100755
--- a/.scripts/build_steps.sh
+++ b/.scripts/build_steps.sh
@@ -34,7 +34,7 @@ CONDARC
 mamba install --update-specs --yes --quiet --channel conda-forge --strict-channel-priority \
     pip mamba conda-build boa conda-forge-ci-setup=3 "py-lief<0.12"
 mamba update --update-specs --yes --quiet --channel conda-forge --strict-channel-priority \
-    pip mamba conda-build boa conda-forge-ci-setup=3 "py-lief<0.12"
+    pip mamba conda-build boa conda-forge-ci-setup py-lief

 # set up the condarc
 setup_conda_rc "${FEEDSTOCK_ROOT}" "${RECIPE_ROOT}" "${CONFIG_FILE}"
diff --git a/.scripts/run_osx_build.sh b/.scripts/run_osx_build.sh
index d47ba22..7a0e943 100755
--- a/.scripts/run_osx_build.sh
+++ b/.scripts/run_osx_build.sh
@@ -26,7 +26,7 @@ conda activate base
 mamba install --update-specs --quiet --yes --channel conda-forge --strict-channel-priority \
     pip mamba conda-build boa conda-forge-ci-setup=3 "py-lief<0.12"
 mamba update --update-specs --yes --quiet --channel conda-forge --strict-channel-priority \
-    pip mamba conda-build boa conda-forge-ci-setup=3 "py-lief<0.12"
+    pip mamba conda-build boa conda-forge-ci-setup py-lief

diff --git a/.scripts/run_win_build.bat b/.scripts/run_win_build.bat
index c4486d9..d25a51f 100644
--- a/.scripts/run_win_build.bat
+++ b/.scripts/run_win_build.bat
@@ -20,7 +20,7 @@ call activate base

 :: Provision the necessary dependencies to build the recipe later
 echo Installing dependencies
-mamba.exe install "python=3.10" pip mamba conda-build boa conda-forge-ci-setup=3 -c conda-forge --strict-channel-priority --yes
+mamba.exe install "python=3.10" pip mamba conda-build boa conda-forge-ci-setup=3 "py-lief<0.12" -c conda-forge --strict-channel-priority --yes
 if !errorlevel! neq 0 exit /b !errorlevel!

 :: Set basic configuration

The last hunk for windows is actually correct - I hadn't cared about that in the manual fix-ups because the constraints are only necessary on osx -, but it's definitely wrong for linux/osx.

h-vetinari commented 1 year ago

Looking slightly above the diff, the constraint is maintained for the original installation, but then discarded for the mamba update, whereupon the previous constraint will be ignored (AFAIU).

jaimergp commented 1 year ago

Ugh, we are now talking about solver behavior implementation details. Whether the history should be a pin or not. I am not even sure why we need the update command to begin with. This all should be more explicit and not as subject to implicit details.

We can fix it, but I think we also need to rethink how we handle the "base environment provisioning" problem.

beckermr commented 1 year ago

History is related to solver details between mamba and conda. Lots of back and forth, plus testing. We should hack around it for now since the underlying behavior differences have not improved.

h-vetinari commented 1 year ago

I noticed that windows does not have the mamba update invocation. It just installs once.

beckermr commented 1 year ago

That is an oversight I think? @isuruf do you recall?

isuruf commented 1 year ago

mamba update is not needed in windows, because we install from scratch whereas on Linux, the docker image might be old.

h-vetinari commented 1 year ago

Ugh, we are now talking about solver behavior implementation details.

Is this really an area of differences between conda & mamba? AFAIU, having an update step without a pin will always try to update that package again. Given the necessity of dealing with stale packages in the docker image, I think we'd have to do one of:

beckermr commented 1 year ago

We did a bunch of testing to settle on the procedure we have. Swapping the order of the calls will break things I think.

jaimergp commented 1 year ago

Is this really an area of differences between conda & mamba? AFAIU, having an update step without a pin will always try to update that package again.

1774 implements the immediate fix. Note it only works with mamba, since conda update doesn't allow constraints in the specs. I think I tested for that precise gotcha before submitting (and then closing), but I'll double check. The history handling seemed to be the difference, IIRC.

If the intent here is to pin packages, we should add an option for that explicitly (base_pins?). I can put a PR tomorrow.

h-vetinari commented 1 year ago

since conda update doesn't allow constraints in the specs

In terms of explicit mechanisms, conda at least supports

    # ensure constraint is respected by each call to conda install/update
    echo "$constraint" > /opt/conda/conda-meta/pinned  # or wherever the main conda config lives

Not sure if mamba supports /conda-meta/pinned, but at least there'd be a way to explicitly express that constraint also for the conda-side.

jaimergp commented 1 year ago

Both mamba and conda respect the pinned file AFAIK. There's also pinned_packages in .condarc and `CONDA_PINNED_PACKAGES=pkg1&pkg2" as an env var.

Let's see how it looks like in a PR.

jaimergp commented 1 year ago

See https://github.com/conda-forge/conda-smithy/pull/1776

isuruf commented 1 year ago

Ugh, we are now talking about solver behavior implementation details.

The update command was needed only for mamba for some weird issue we saw earlier. So, it's fine to just do that for mamba. There's no need to support that for conda.

jaimergp commented 1 year ago

The update command was needed only for mamba for some weird issue we saw earlier. So, it's fine to just do that for mamba. There's no need to support that for conda.

Oh nice, I can add one more clause to that if. Thanks!