merge c_stdlib_version & MACOSX_DEPLOYMENT_TARGET on osx

[h-vetinari]

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

Closes #1884

[h-vetinari]

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]

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]

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

[h-vetinari]

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

Opened conda-forge/conda-forge-pinning-feedstock#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]

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]

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 conda-forge/conda-forge-pinning-feedstock#5669, but at least then it'll be a pretty obvious error, rather than a mysterious failure because we're changing zips mid-rerender.

[beckermr]

We should test this with the new pinnings file in that pr on a live feedstock to ensure it all works. Otherwise LGTM and thank you!

[h-vetinari]

OK, 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:

• the mismatch warning gets displayed a stunning number of times (768 times on the abseil feedstock); we should reduce that to displaying the warning only once IMO
• even a vanilla setup following the warnings (i.e. setting only c_stdlib_version and not MACOSX_DEPLOYMENT_TARGET in the local cbc) runs into the discrepancy between local c_stdlib_version and global MACOSX_DEPLOYMENT_TARGET. This case should clearly not warn though - here we can either decide to remove MACOSX_DEPLOYMENT_TARGET from the global pinning as soon as we release smithy after this PR, or we need to build more logic to determine what's in the local CBC.

[h-vetinari]

@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]

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

[beckermr]

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

[beckermr]

See here: conda-forge/conda-forge-pinning-feedstock#5669 (comment)

[h-vetinari]

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 conda-forge/conda-forge-pinning-feedstock#5672; conda-forge/conda-forge-pinning-feedstock#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]

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

How could I reproduce this?

[h-vetinari]

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]

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

• after the call to _get_used_key_values_by_input_order,
• with also modifying a copy(!) of list_of_metas[0].config.input_variants and that for squished_input_variants then, or
• far earlier when the MetaData is created at https://github.com/h-vetinari/conda-smithy/blob/53ed894e6532ccd63afecc0be41e5114cabaad75/conda_smithy/configure_feedstock.py#L953-L983 if you'd want to modify the meta.config.input_variants directly.

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]

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]

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]

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]

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

[h-vetinari]

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]

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!