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

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

Closed
h-vetinari opened this issue Sep 26, 2023 · 21 comments · Fixed by #1774 or #1775
Closed

Comments

@h-vetinari
Copy link
Member

It seems that 4a5ac16 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

config["remote_ci_setup_names"] = [
MatchSpec(pkg).name for pkg in config["remote_ci_setup"]
]

using .name unconditionally.

This has effectively made compiler-rt un-rerenderable (except with manual fixes), because we cannot remove the py-lief pin, since conda/conda-build#4787 was only fixed in conda-build 3.26, which we still cannot pull into our CI due to conda-forge/boa-feedstock#75. For a sample CI failure without the pin, see conda-forge/compiler-rt-feedstock#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 <module>
    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 <listcomp>
    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 <genexpr>
    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
Copy link
Member

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
Copy link
Member Author

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 conda/conda-build#4787) only occurs on osx, so that's where we'd mainly need it.

@jaimergp
Copy link
Member

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

@jaimergp
Copy link
Member

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
Copy link
Member Author

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

@h-vetinari
Copy link
Member Author

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
Copy link
Member Author

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) conda-forge/compiler-rt-feedstock#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 h-vetinari reopened this Sep 27, 2023
@h-vetinari
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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

@beckermr
Copy link
Member

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

@isuruf
Copy link
Member

isuruf commented Sep 27, 2023

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

@h-vetinari
Copy link
Member Author

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:

  • also propagate the constraints to mamba update (as was done for conda-smithy <3.26)
  • switch the order of mamba install and mamba update; that way, we could update the image first, and then install whatever constraints we need on top. Not sure this is 100% free of solver-traps, so the first option is probably safer.

@beckermr
Copy link
Member

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
Copy link
Member

jaimergp commented Sep 28, 2023

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

See #1776

@isuruf
Copy link
Member

isuruf commented Sep 28, 2023

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
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants