h-vetinari commented 3 months ago

Tentative fix for #1911, needed for https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/5790 (AFAIU), tested in https://github.com/conda-forge/scipy-feedstock/pull/274 and https://github.com/conda-forge/pythran-feedstock/pull/84

beckermr commented 3 months ago

Can you add a test recipe for this one? Without a simplified example, I won't be able to understand what changed. Further, this issue seems easy to miss and like a thing we don't want to regress later.

h-vetinari commented 2 months ago

I've now added a test based on the current state of https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/5790, which works fine for feedstocks on py>=39. For py38 we'll need some further fixes, but this one will be necessary in any case.

Without a simplified example, I won't be able to understand what changed.

I've reproduced the current situation (i.e. global pinning + 3 affected migrators for pypy, py312 & numpy2); though I guess the failure would also show up with one less migrator. Without https://github.com/conda-forge/conda-smithy/pull/1912/commits/3ecc8c4fb4126c28341ffbc0cd3eff99931398e5, the rerender wrongly produces

channel_sources:
- conda-forge

AFAICT this happens because the rerender will produce a list ["conda-forge", "conda-forge/label/numpy_dev,conda-forge"] after merging, but we always just take the 0th element since conda build needs a string for the channel_sources.

It's not obvious how to broadcast these two elements to the 6 we need here, so the least we can do is update all of them consistently (in the migrators, and take that into account in smithy). The next step would be using additional_zip_keys in all three migrators, but that has two problems:

AFAICT the current code is not ready for using the same additional_zip_keys in two migrations (because once the first one has been applied, the new key already is part of the zip)
additional_zip_keys does not apply to kind: version migrators (or, more precisely, while we can add operation: key_add to end up calling op_variant_key_add, this does not work for a baseline version update). I've hacked around on this and couldn't get it to work.

beckermr commented 2 months ago

AFAICT this happens because the rerender will produce a list ["conda-forge", "conda-forge/label/numpy_dev,conda-forge"] after merging, but we always just take the 0th element since conda build needs a string for the channel_sources.

For channel sources, shouldn't we combine the strings at some point before the final ci support files are made?

h-vetinari commented 2 months ago

So it appears that the last migrator applied will win if two or migrators override the same non-zip key.

AFAICT it's quite unusual that several migrators are overriding the same key? Basically, this only happens if several different migrators tackle different components of a big zip, as happens with us for python 3.12, pypy & numpy. I don't think there are many other scenarios where this would become relevant, but perhaps I'm not creative enough...

I kind of think we should punt on this because for now it's mostly a hypothetical concern (compare the unhandled corner-cases for additional_zip_keys). An alternative would be to raise a warning if we spot an inconsistency (i.e. a key not part of a zip that has length >1 after merging, assuming that's a good metric).

For channel sources, shouldn't we combine the strings at some point before the final ci support files are made?

They already need to be provided in combined form in the recipes CBC, c.f.

conda-forge/label/numpy_dev,conda-forge
^                           ^
1                           2

beckermr commented 2 months ago

We should not leave this quirk / bug in the code just because we think it is rare.

For the channel sources, I am saying that when smithy produces CBCs like this

channel_sources:
  - blah
  - blah_blah

it should automatically convert them to single strings

channel_sources:
  - blah,blah_blah

h-vetinari commented 2 months ago

it should automatically convert them to single strings

That sounds really dangerous IMO. So far the entire channel_sources (plural!) logic is based on providing this value as a block. Having

channel_sources:
  - blah
  - blah_blah

is almost certainly a sign of something gone wrong in the variant merge and should IMO warn/raise, not silently merge. This kind of merge would almost certainly create issues with strict channel priority too.

beckermr commented 2 months ago

If we refuse to merge them automatically since it is a sign of an issue and so smithy should error loudly if it ever produces more than one value, wouldn't that preclude the code fixup here since this PR is intended to fix a case where smithy did produce more than one value but chose the wrong one?

beckermr commented 2 months ago

In other words, how do I square the "raise on multiple values instead merging them" logic with this

AFAICT this happens because the rerender will produce a list ["conda-forge", "conda-forge/label/numpy_dev,conda-forge"] after merging, but we always just take the 0th element since conda build needs a string for the channel_sources.

h-vetinari commented 2 months ago

wouldn't that preclude the code fixup here since this PR is intended to fix a case where smithy did produce more than one value but chose the wrong one?

I'm not sure I understand. Before https://github.com/conda-forge/conda-smithy/pull/1912/commits/3ecc8c4fb4126c28341ffbc0cd3eff99931398e5, the merge produced two values but ignored the second. With this PR it is at least able to produce a single value (provided the migrators are consistent between themselves).

In other words, how do I square the "raise on multiple values instead merging them" logic with this

This was trying to describe the problem before this PR.

beckermr commented 2 months ago

When I suggested that we automatically merge the keys, you told me that

That sounds really dangerous IMO. So far the entire channel_sources (plural!) logic is based on providing this value as a block. [...] is almost certainly a sign of something gone wrong in the variant merge and should IMO warn/raise, not silently merge. This kind of merge would almost certainly create issues with strict channel priority too.

The PR here is fixing the exact case of multiple values by picking one. This PR does not raise a warning or an error as you advocated above.

The options appear to be:

pick one of the values
raise a warning
raise an error
merge them automatically

Which option do you think we should do?

h-vetinari commented 2 months ago

The PR here is fixing the exact case of multiple values by picking one.

That's a misunderstanding I think. This PR is fixing it by ensuring there is only one. It does this by always updating the value if a migrator specifies it, which is not exactly the same as picking one of the values, because it produces the correct result in the case of non-racy migrator specs.

The race condition argument is a fair criticism though, which we could try to warn on.

beckermr commented 2 months ago

I am confused sorry. We already specify a key in the global pinnings, so any migrator setting anything creates more than one. I don't understand why the first one set is special relative to the rest.

I don't like putting a partial solution into smithy and leaving edge cases for us to have to go back and figure out later.

We should ensure this works in the general case now instead of taking the fast path to get one feature moving.

h-vetinari commented 2 months ago

Let's take a step back. Our migrator infrastructure is supposed to help us go from:

foo:
  - 1

with

__migrator:
  kind: version
foo:
  - 2

to

foo:
  - 2

That is the absolute core functionality. In general this works well, being able to update multiple keys, or a component of a zip, or even all components of a zip. But it fails if the migrator updates a zip plus something else. I've reduced the test here accordingly in https://github.com/conda-forge/conda-smithy/pull/1916, and it fails on main. IMO this must be considered a bug and so fixing it is not a feature.

However, I note that my proposed fix here does not fix the test in #1916, so I guess we can take that as net information gain from this discussion (I'm the first to admit that I have a hazy understanding of the smithy codebase at best 😅).

beckermr commented 2 months ago

Yes indeed you have identified the core thing. My point is that this fix is only partial and I think we should take the time to develop something that won't suffer from race conditions and deals properly or raises an error for channels. (A warning is not going to be visible enough given how our tools currently work.)

h-vetinari commented 2 months ago

something that won't suffer from race conditions

1916 has only a single migration, so there's nothing to race. If we can fix that without opening other cans of worms (unrelated to this PR; as I mentioned, the fix here doesn't solve #1916), that should be an unequivocal win, right?

beckermr commented 2 months ago

We'll start another python soon enough and then run right into this as the numpy one drags on for a while. Really I don't get why we are putting in a partial fix. Let's get this done while we are here so we don't have to come back later. If you are not willing, then i'll happily take this on but it may be a bit.

h-vetinari commented 2 months ago

Really I don't get why we are putting in a partial fix.

I did say that the fix I'm advocating is for #1916 first and foremost, and unrelated to this PR. How that fix will look is unknown for now (too late for me to continue investigating last night), so I'm not sure where you get the "partial fix" from. Let's judge the implementation when one is available?

beckermr commented 2 months ago

I mean this PR is a partial fix for the issue it is claiming to fix as I detailed above.

h-vetinari commented 2 months ago

I mean this PR is a partial fix for the issue it is claiming to fix as I detailed above.

Yes, and I already said I don't care about this PR, but about solving the underlying issue, which by then had been reduced to #1916:

@h-vetinari: If we can fix that [#1916] without opening other cans of worms (unrelated to this PR; as I mentioned, the fix here doesn't solve #1916), that should be an unequivocal win, right?

In fact, I'm going to close this PR, because I figured out the fix for #1916, and it doesn't even need changes in smithy.

conda-forge / conda-smithy

Do not ignore zip-unrelated keys in migrators #1912

1916 has only a single migration, so there's nothing to race. If we can fix that without opening other cans of worms (unrelated to this PR; as I mentioned, the fix here doesn't solve #1916), that should be an unequivocal win, right?