search

conda-forge / conda-smithy

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

merge c_stdlib_version & MACOSX_DEPLOYMENT_TARGET on osx #1889

Closed h-vetinari closed 4 months ago

h-vetinari commented 4 months ago

~Builds on #1888 (draft until that's merged)~

Closes #1884

h-vetinari commented 4 months ago

Other than the discussion that already started, AFAICT this should do what we discussed in #1884 (warns in case of mismatch, uses max to populate both values), as confirmed also by the test.

h-vetinari commented 4 months ago

Pulling out a comment from the review thread

Also, we should be able to handle zip keys there too by inspecting them and adding MACOSX_DEP... to the group if c_stdlib_version shows up in it.

I'm not sure I want to deal with having to insert something into the mega-zip involving c_stdlib_version. That sounds like opening a massive can of worms to me, unless we add MACOSX_DEPLOYMENT_TARGET to that zip in the global pinning itself. Otherwise we'd provoke mismatched zip length way too easily IMO, all while rerendering (so extremely hard to diagnose/debug).

Though I guess adding MACOSX_DEPLOYMENT_TARGET to the zip shouldn't be a big issue, as it exactly encodes the same information as c_stdlib_version.

beckermr commented 4 months ago

Yep. The keys are one to one so inserting should be safe.

h-vetinari commented 4 months ago

Yep. The keys are one to one so inserting should be safe.

Opened https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/5669.

Coming back to

Also, we should be able to handle zip keys there too by inspecting them and adding MACOSX_DEP... to the group if c_stdlib_version shows up in it.

I think this is now unnecessary, as in practice (i.e. when the global pinning is in use), there'll always be a zip, and both c_stdlib_version & MACOSX_DEPLOYMENT_TARGET are part of it already.

beckermr commented 4 months ago

Ok. I don't understand the trade offs between these approaches but it appears it should work.

We'll want some more eyes on this likely.

h-vetinari commented 4 months ago

Ok. I don't understand the trade offs between these approaches but it appears it should work.

The risk of not adding this to the relevant zip in the global pinning is that your previously suggested approach of adding MACOSX_DEPLOYMENT_TARGET to an existing zip would break if any recipe does something like:

MACOSX_DEPLOYMENT_TARGET:   # [osx and x86_64]
  # compat build
  - 10.9                    # [osx and x86_64]
  # build using newer features
  - 11.0                    # [osx and x86_64]
  - 11.0                    # [osx and arm64]

because then all the keys participating in the zip would need to be adapted to match the length (or we would have to dynamically infill that, which sounds even more fragile). That problem does not disappear with https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/5669, but at least then it'll be a pretty obvious error, rather than a mysterious failure because we're changing zips mid-rerender.

h-vetinari commented 4 months ago

OK, while testing this (together with https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/5672), I ran into two issues that aren't critical but IMO need to be fixed:

h-vetinari commented 4 months ago

@beckermr: We should test this with the new pinnings file in that pr on a live feedstock to ensure it all works.

I did that; the ucx-split feedstock that previously had issues with the pinning works fine, and so does another random one where I experimented setting/not setting c_stdlib_version in the CBC (abseil).

@h-vetinari: while testing this (together with conda-forge/conda-forge-pinning-feedstock#5672), I ran into two issues that aren't critical but IMO need to be fixed:

Both these issues are now fixed too; I'm not really sure how to write a test for the second one because it needs two config files at play (the global pinning vs. the local CBC), but I've tested it successfully.

h-vetinari commented 4 months ago

In the context of https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/5672, I tested local rerenders with the new pinning and a smithy built from this PR, and things work fine.

beckermr commented 4 months ago

According to that pr we're supposed to add an additional zip of the deployment target.

beckermr commented 4 months ago

See here: https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/5669#issuecomment-2016923689

h-vetinari commented 4 months ago

According to that pr we're supposed to add an additional zip of the deployment target.

OK, retesting with that added zip now breaks this PR, for reasons that escape me:

  File "[...]\dev\conda-forge\conda-smithy\conda_smithy\configure_feedstock.py", line 615, in _collapse_subpackage_variants
    used_key_values = conda_build.variants.list_of_dicts_to_dict_of_lists(
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "E:\miniforge\envs\smithy-dev\Lib\site-packages\conda_build\variants.py", line 634, in list_of_dicts_to_dict_of_lists
    values = list(zip(*set(zip(*(squished[key] for key in group)))))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "E:\miniforge\envs\smithy-dev\Lib\site-packages\conda_build\variants.py", line 634, in <genexpr>
    values = list(zip(*set(zip(*(squished[key] for key in group)))))
                                 ~~~~~~~~^^^^^
KeyError: 'c_stdlib_version'

I've tried debugging this locally, and it seems to be going wrong after _get_used_key_values_by_input_order; by then the used_key_values dict contains basically (trimmed):

{'MACOSX_DEPLOYMENT_TARGET': [],
 'c_compiler': ['clang'],
 'c_compiler_version': ['16'],
 'c_stdlib': ['macosx_deployment_target'],
 'c_stdlib_version': [],
 'cxx_compiler': ['clangxx'],
 'cxx_compiler_version': ['16'],
 'target_platform': ['osx-64'],
 'zip_keys': [('c_stdlib_version', 'MACOSX_DEPLOYMENT_TARGET')]}

In other words, the values for c_stdlib_version/MACOSX_DEPLOYMENT_TARGET are an empty list, which (AFAIU) later gets filtered out, which is why we eventually hit that KeyError.

Looking at the function _get_used_key_values_by_input_order, there's a distinction for handling values that are part of a zip and those that aren't (c_stdlib_version isn't part of the zip for osx as of https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/5672; https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/5669 readds it).

Long story short, we're not robust to (all flavours of) zipping here yet, and I'm not sure exactly what's going wrong.

mbargull commented 4 months ago

OK, retesting with that added zip now breaks this PR, for reasons that escape me:

How could I reproduce this?

h-vetinari commented 4 months ago

How could I reproduce this?

Download the conda_build_config.yaml from that PR and save it somewhere, say pinning.yaml, and put it (for example) one folder out from the base of a given feedstock.

Then install a local version of smithy (from this PR) into an environment and run the following from the base of your favourite feedstock.

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

I used abseil-cpp, and updated the CBC in line with the "new way" of doing things:

c_stdlib_version:  # [osx and x86_64]
  - "10.13"        # [osx and x86_64]
mbargull commented 4 months ago

The issue is essentially that you modify the local all_variants set for which its values (then in squished_used_variants) get compared to values from list_of_metas[0].config.input_variants (then in squished_input_variants). We expect these values (but not their order) to match to be able to restore their input order in _get_used_key_values_by_input_order. Meaning, you'd either have to do such modifications

I did not look into which of these options makes the most sense; you'll have to check which causes the least (potential) inconsistencies.

h-vetinari commented 4 months ago

Yeah, I had noted while debugging that squished_input_variants had wrong (=un-updated) values, but didn't see right away how that breaks the logic.

Thanks for taking a look! I think the easiest might just be a manual fix-up of squished_input_variants...

mbargull commented 4 months ago

I think the easiest might just be a manual fix-up of squished_input_variants...

Likely easiest, yes. Looking at this again with more context than just the code diff here, I'd say, given the function name and that (AFAICT) we don't modify other actual values therein, it is probably not best conceptually or with future maintenance in mind. It likely makes more sense to put this outside/before of the dump_subspace_config_files call chain (which means, we'd be back at https://github.com/h-vetinari/conda-smithy/blob/53ed894e6532ccd63afecc0be41e5114cabaad75/conda_smithy/configure_feedstock.py#L953-L983 at the latest).

h-vetinari commented 4 months ago

Meaning, you'd either have to do such modifications [...] with also modifying a copy(!) of list_of_metas[0].config.input_variants

I took an approach that roughly follows this and is (I believe) not a terrible hack. It works now with the abseil example that failed previously 🥳

PTAL! :)

beckermr commented 4 months ago

@h-vetinari is there a feedstock and/or recipe we should test on live before merging?

h-vetinari commented 4 months ago

I've tested this on ucx-split (had problems with stdlib-zip plus CUDA before), also cupy, abseil (basic C++ case; with various configurations of local CBC) and numpy (python package).

Happy to test this elsewhere if someone has a gnarly feedstock in mind, but I think things should be ready to merge now.

mbargull commented 4 months ago

I took an approach that roughly follows this and is (I believe) not a terrible hack. It works now with the abseil example that failed previously 🥳

Since Matt is fine with it, I'll take it :). Since functionality-wise we have everything we need, I think it is good that you went ahead and started on the first migration already. Thanks for your commitment on this!