conda-forge/conda-forge-repodata-patches-feedstock

ENH: support removal of upper bounds in yaml patching

Closed this issue · 11 comments

h-vetinari commented

I'm in a situation (#879) where I need to loosen a dependency from a run-export, but not just replace the upper bound, but remove it completely. In reference to the API of pin_subpackage, I had written this as:

  - loosen_depends:
      name: libre2-11
      upper_bound: None

however, that doesn't work as intended. None gets interpreted as a string and so we get

linux-64::libarrow-13.0.0-h5534346_26_cpu.conda
-    "libre2-11 >=2023.6.2,<2024.0a0",
+    "libre2-11 >=2023.6.2,<None.0a0",

I'm not sure what the best way to implement this would be. Using "None" as a value seems fragile, because the processing for "loosen_depends" is full of None handling already. Perhaps we could a magic cookie like "upper_bound: |remove|"?

This approach would be following the current requirement of "you must give one of max_pin or upper_bound " outlined in the readme for loosen_depends. A more invasive alternative would be to introduce a third option next to max_pin and upper_bound, e.g. remove: which can be either "upper" or "lower" (though I note that {tighten,loosen}_depends don't really support handling of lower bounds anyway).

CC @beckermr

h-vetinari commented

Thoughts on this @beckermr?

beckermr commented

Using an a branch on None will be fine as long as we branch right at the top instead of trying to do it throughout the function. Should be a straight-forward PR and the tests should be easy enough to write to catch all of the edge cases. PRs welcome!

h-vetinari commented

Using an a branch on None will be fine

But it won't be None, it'll be "None", because yaml can only produce strings/ints/floats, not None. And that distinction is IMO way too easy to confuse, so rather than "None", I think we should take a more recognizably magic cookie like "|remove|" or "<remove>".

beckermr commented

The value null will produce none. The actual cases are not that hard to enumerate and are easier than making people remember a special value they have not seen before.

h-vetinari commented

so IIUC you propose that people write the following in the yamls:

  - loosen_depends:
      name: libre2-11
      upper_bound: null  # but not None

easier than making people remember a special value they have not seen before.

To my mind, null fall exactly under that category as well, but 🤷

h-vetinari commented

I added something in #879. It seems to work, but I'm not sure what you had in mind, esp. w.r.t. testing.

beckermr commented

We can support either null or text None. It is a simple or statement. No need to force folks into one or the other.

beckermr commented

Also null is not random. Yaml is a superset of json and "null" is part of the json standard. See here https://www.json.org/json-en.html

h-vetinari commented

I was responding to the formulation "special value they have not seen before." I haven't seen null in any of our yamls - not arguing the point that it's not "official"; otherwise the transformation to None wouldn't work.

We can support either null or text None. It is a simple or statement. No need to force folks into one or the other.

I put in an error for the "None" case. I see no reason to allow two separate ways to do this, where one is additionally very confusing. Once a user gets the error, it's easy to switch to null IMO.

beckermr commented

Ok. I'll back out my commit then. I don't actually care here but I dont want a value that we have completely made up.

h-vetinari commented

I don't feel super strongly about supporting "None" or not - it was the thing I reached for first after all - so I left your commit in but removed the mixed-case variants.

  • 👍1