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

Stdlib: c_stdlib_version falsely determined as unset #5210

Closed
h-vetinari opened this issue Mar 3, 2024 · 10 comments
Closed

Stdlib: c_stdlib_version falsely determined as unset #5210

h-vetinari opened this issue Mar 3, 2024 · 10 comments
Labels
pending::support indicates user is waiting on support from triage engineers severity::2 critical; broken functionality with an unacceptably complex workaround source::community catch-all for issues filed by community members type::support neither a bug nor feature, is really just a user having questions or difficulty somewhere

Comments

@h-vetinari
Copy link
Contributor

While trying to rerender https://github.com/conda-forge/ctng-compiler-activation-feedstock with the newest conda-build, I'm running into a problem that's seemingly related to the new stdlib-jinja. However, that recipe does not use this function at all...?

The error looks as follows:

(builder) [...]\conda-forge\ctng-compiler-activation-feedstock>conda smithy rerender
INFO:conda_smithy.configure_feedstock:README rendering is skipped
INFO:conda_smithy.configure_feedstock:__pycache__ rendering is skipped
WARNING: Setting build platform. This is only useful when pretending to be on another platform, such as for rendering necessary dependencies on a non-native platform. I trust that you know what you're doing.
WARNING: Setting build arch. This is only useful when pretending to be on another arch, such as for rendering necessary dependencies on a non-native arch. I trust that you know what you're doing.
WARNING: No numpy version specified in conda_build_config.yaml.  Falling back to default numpy value of 1.23
Adding in variants from internal_defaults
Adding in variants from C:\Users\[...]\AppData\Local\Temp\conda-smithy\conda_build_config.yaml
Adding in variants from C:\Users\[...]\dev\conda-forge\ctng-compiler-activation-feedstock\recipe\conda_build_config.yaml
Adding in variants from argument_variants
Traceback (most recent call last):
  File "E:\miniforge\envs\builder\Scripts\conda-smithy-script.py", line 9, in <module>
    sys.exit(main())
             ^^^^^^
  File "E:\miniforge\envs\builder\Lib\site-packages\conda_smithy\cli.py", line 724, in main
    args.subcommand_func(args)
  File "E:\miniforge\envs\builder\Lib\site-packages\conda_smithy\cli.py", line 584, in __call__
    self._call(args, tmpdir)
  File "E:\miniforge\envs\builder\Lib\site-packages\conda_smithy\cli.py", line 589, in _call
    configure_feedstock.main(
  File "E:\miniforge\envs\builder\Lib\site-packages\conda_smithy\configure_feedstock.py", line 2697, in main
    render_azure(env, config, forge_dir, return_metadata=True)
  File "E:\miniforge\envs\builder\Lib\site-packages\conda_smithy\configure_feedstock.py", line 1664, in render_azure
    return _render_ci_provider(
           ^^^^^^^^^^^^^^^^^^^^
  File "E:\miniforge\envs\builder\Lib\site-packages\conda_smithy\configure_feedstock.py", line 828, in _render_ci_provider
    ) = conda_build.variants.get_package_combined_spec(
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "E:\miniforge\envs\builder\Lib\site-packages\conda_build\variants.py", line 665, in get_package_combined_spec
    validate_spec(f, spec)
  File "E:\miniforge\envs\builder\Lib\site-packages\conda_build\variants.py", line 192, in validate_spec
    raise ValueError(
ValueError: Variant configuration errors in C:\Users\[...]\AppData\Local\Temp\conda-smithy\conda_build_config.yaml:
  zip_key entry c_stdlib_version in group frozenset({'c_stdlib_version', 'c_stdlib'}) does not have any settings

Aside from the recipe not using {{ stdlib("c") }}, the specification is also definitely present already in conda-forge's global pinning.

I've tried monkey-patching my conda-build to contain #5195, as well as adding the stdlib-config to the local CBC, and even setting some value for the windows c_stdlib_version (that's not set in the global pinning because it shouldn't be necessary). In all cases, I get the same error.

The error appears in

# check for properly formatted zip_key
try:
zip_keys = _get_zip_keys(spec)
except ValueError as e:
errors.append(str(e))
else:
# check if every zip field is defined
errors.extend(
f" zip_key entry {k} in group {zg} does not have any settings"
for zg in zip_keys
for k in zg
# include error if key is not defined in spec
if k not in spec
)

but I can't quite yet tell why that would be the case.

CC @mbargull @beckermr

@h-vetinari h-vetinari changed the title Stdlib: c_stdlib_version Stdlib: c_stdlib_version falsely determined as unset Mar 3, 2024
@beeankha beeankha added this to the 24.3.x milestone Mar 4, 2024
@beeankha
Copy link
Contributor

beeankha commented Mar 4, 2024

Hi @h-vetinari , I've added this to the 24.3.x release milestone list; would you consider this issue a blocker (i.e. conda-build should be considered broken or will significantly block conda-forge-related work etc.) if this isn't fixed in time to be included in the March release?

@h-vetinari
Copy link
Contributor Author

h-vetinari commented Mar 4, 2024

It depends a bit on the fallout. It's strange to me that a recipe that's not using any of the new functionality would stumble while being rendered - for that recipe, the outcome is quite bad, because we cannot change configuration etc., without doing very error-prone manual surgery.

So far I've just encountered one that failed to rerender, which I wouldn't consider a blocker. But if it's more widespread, then yes.

In any case, in the context of some urgency for the upgrades of glibc and macos deployment target this year, any bugs in the stdlib functionality have high priority to me, especially if the lead times accumulate (but I'll admit that my priorities are perhaps not 100% representative of everyone else 😅)

@beckermr
Copy link
Contributor

beckermr commented Mar 4, 2024

I doubt this is a one-off case and I think this is a blocker for the release. My worry is about the conda-build test suite. How did this bug leak through?

@kenodegard kenodegard added type::support neither a bug nor feature, is really just a user having questions or difficulty somewhere source::community catch-all for issues filed by community members labels Mar 5, 2024
@kenodegard
Copy link
Contributor

This is not an issue with conda-build. The conda-forge cbc.yml is invalid for linux-s390x.

Using the following recipe/conda_build_config.yaml:

c_stdlib:
  - sysroot                    # [linux]
  - macosx_deployment_target   # [osx]
  - vs                         # [win]
c_stdlib_version:              # [unix]
  - 2.12                       # [linux64]
  - 2.17                       # [aarch64 or ppc64le]
  - 10.9                       # [osx and x86_64]
  - 11.0                       # [osx and arm64]
zip_keys:
  -                             # [unix]
    - c_stdlib                  # [unix]
    - c_stdlib_version          # [unix]

And this recipe/meta.yaml:

package:
  name: conda-forge-cbc
  version: 1.0

build:
  noarch: True
  script:
    - echo hello world

We can reproduce the error:

$ CONDA_SUBDIR=linux-s390x conda build recipe/
...
    ValueError: Variant configuration errors in /Users/kodegard/scratch/conda-forge-cbc/recipe/conda_build_config.yaml:
      zip_key entry c_stdlib_version in group frozenset({'c_stdlib', 'c_stdlib_version'}) does not have any settings

Versus:

$ CONDA_SUBDIR=linux-64 conda build recipe/
...

@kenodegard kenodegard added the pending::feedback indicates we are waiting on feedback from the user label Mar 5, 2024
@h-vetinari
Copy link
Contributor Author

h-vetinari commented Mar 5, 2024

Thanks for the response @kenodegard!

So there's 3-4 different things happening here:

  • a recipe that didn't use anything related to the new stdlib jinja broke. In conda-forge we'll be relatively fine (because closely involved with this change), but people in other channels or proprietary settings might be very confused if their recipes break, and the solution is not obvious (add c_stdlib{,_version} to CBC)
  • the fact that I missed the config for s390x - that's totally on me. In my defence, s390x is so rare that I forget it's presence - that failure mode didn't even occurr to me, sorry! I guess we'll go add something for it to the global pinning (or at least the CBC for the compiler feedstock).
  • even when adding config for s390x, the rerender still fails, because now windows only has a stdlib but not a version. This should be made more lenient I think. Aside from helping out people who're not using stdlib, we shouldn't need to set the version (like we do for the compilers on windows as well)
  • finally, even if I give windows a dummy version, the rerender still fails. I think this might be due to some other rare/obsolete architectures falling into the same trap as s390x; presumably I'm hitting linux-32 here (or something else like armv7l)? This also exacerbates the breakage, because it can happen on architectures that the recipe is skipping...? I've hardened the CBC as follows:
    --- a/recipe/conda_build_config.yaml
    +++ b/recipe/conda_build_config.yaml
    @@ -44,8 +44,8 @@ c_stdlib:
       - vs                         # [win]
     c_stdlib_version:
       - 2.12                       # [linux64]
    -  - 2.17                       # [aarch64 or ppc64le or s390x]
    -  - 10.9                       # [osx and x86_64]
    +  - 2.17                       # [linux and not linux64]
    +  - 10.9                       # [osx and not arm64]
       - 11.0                       # [osx and arm64]
       - 2019                       # [win]
    and still the rerender fails with the same error. I've tried a local CONDA_SUBDIR=<target> conda build recipe/ for all the targets I could think of, and I cannot determine where it's going wrong. The one thing that stood out to me though is that something is messing with the ordering though...
    $ CONDA_SUBDIR=linux-64 conda build recipe/
    [...]
    Attempting to finalize metadata for binutils_linux-64  ✅ 
    $ CONDA_SUBDIR=linux-aarch64 conda build recipe/
    [...]
    Attempting to finalize metadata for binutils_linux-ppc64le  ❌  
    $ CONDA_SUBDIR=linux-ppc64le conda build recipe/
    [...]
    Attempting to finalize metadata for binutils_linux-64  ❌ 
    $ CONDA_SUBDIR=linux-s390x conda build recipe/
    [...]
    Attempting to finalize metadata for binutils_linux-s390x  ✅ 
    $ CONDA_SUBDIR=linux-armv7l conda build recipe/
    [...]
    Attempting to finalize metadata for binutils_linux-ppc64le  ❌
    

IMO the consequences for conda-build should be, at the very least:

  • don't fail on missing c_stdlib unless {{ stdlib("c") }} is used in the recipe for that architecture - IMO this should be considered a regression.
  • don't fail on missing c_stdlib_version in any case; it's something we can work around but shouldn't be necessary. Just don't insert a version pin after what's inserted through c_stdlib.

@conda-bot conda-bot added pending::support indicates user is waiting on support from triage engineers and removed pending::feedback indicates we are waiting on feedback from the user labels Mar 5, 2024
@jezdez jezdez added ¡blocking! used to indicate a blocker for a pending release severity::2 critical; broken functionality with an unacceptably complex workaround labels Mar 5, 2024
@beckermr
Copy link
Contributor

beckermr commented Mar 5, 2024

I did more digging here:

  • You have to run conda-smithy very carefully in order to not use the global conda-forge conda-build config.

  • I made a bug fix here: BUG fix zipping for c stdlib conda-forge/conda-forge-pinning-feedstock#5592

  • If I run this command

    conda-smithy rerender --no-check-uptodate -e ../conda_forge_pinning.yaml
    

    on the binutils-feedstock it rerenders fine locally.

  • Edit: I fixed the zipping with the cdt keys too. I do see something off with the c_stdlib_version for linux-64 in the rerender. However, I think this is due to conda-forge's setup and not conda-build itself.

@kenodegard
Copy link
Contributor

kenodegard commented Mar 5, 2024

  • don't fail on missing c_stdlib unless {{ stdlib("c") }} is used in the recipe for that architecture - IMO this should be considered a regression.

None of this is specific to stdlib. If we replace c_stdlib and c_stdlib_version with foo and bar we will get the same error.

I agree that we shouldn't necessarily fail when keys are incorrectly defined in cbc.yml and also unused in the recipe at hand but I'd also argue that we shouldn't allow usage of a broken cbc.yml and should handle it ASAP.

It would seem to me that it would be better for smithy to intercept the error and say something like "cbc.yml is broken for ____ platform".

  • don't fail on missing c_stdlib_version in any case; it's something we can work around but shouldn't be necessary. Just don't insert a version pin after what's inserted through c_stdlib.

The error being thrown is valid. We've specified a zip_key but one or more of the keys are undefined (after filtering on the selectors). That'd be the same as saying

for stdlib, version in zip(c_stdlib, c_stdlib_version):
    print(stdlib, version)

and then wondering why nothing gets printed.

What you're suggesting is that zip_keys is modified to work like zip_longest instead of zip and I'm very certain doing so will have unintended consequences elsewhere.

Would it help if conda-build offered a subcommand to verify the cbc.yml for some list of platforms?

@beckermr
Copy link
Contributor

beckermr commented Mar 5, 2024

Thanks @kenodegard! Yes I agree with what you are saying above.

Here is my suggestion for how to proceed:

  • I merged the pinning update PR above.
  • Once that package is live, let's retest the rendering issues found by @h-vetinari. If they go away, then I think we can proceed with a conda-build release.
  • At the same time, let's make some new issues to track some of your ideas in the discussions here. These could include improving the error reporting and providing a command to validate the CBC. I am a bit weary of suppressing errors with unused keys for the reasons mentioned above, namely that is an actual bug and might indicate other issues (like it did here!).

@h-vetinari
Copy link
Contributor Author

Thanks for the inputs and the pinning PR! With an unmodified recipe and the new pinning, the ctng-compiler-activation now rerenders again.

@beeankha beeankha removed the ¡blocking! used to indicate a blocker for a pending release label Mar 6, 2024
@beeankha beeankha modified the milestones: 24.3.x, 24.5.x Mar 6, 2024
@beeankha
Copy link
Contributor

Per @beckermr 's comment here, we opened a new issue to track further improvements for cbc.yml error reporting and validations. Closing this ticket since the original issue discussed here has been resolved.

@beeankha beeankha removed this from the 24.5.x milestone Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending::support indicates user is waiting on support from triage engineers severity::2 critical; broken functionality with an unacceptably complex workaround source::community catch-all for issues filed by community members type::support neither a bug nor feature, is really just a user having questions or difficulty somewhere
Projects
Archived in project
Development

No branches or pull requests

6 participants