CEP 39 - A new recipe format (part 3): jinja functions in recipes - #71
Conversation
|
In conda/conda-build#5053 you had said:
Is this that CEP? In any case, I don't see |
|
Yep, this is the one. Do you want to have it as a separate function or rolled into |
I'd prefer a separate function, because it's less magic to have to understand (i.e. "why do I need to specify |
|
My points:
I can imagine two behaviors:
Note that while you are correctly pointing out a dislike about magical behavior, we are printing a lot more information, clearly displayed, with rattler-build. So we can make it obvious where the And lastly, I just think it's a matter of writing really good documentation. |
Define "work". When we increase the It would only affect very old systems, but I'd still consider that broken.
For me it's the opposite here - a compiled recipe needs (in 99.9% of cases) a C standard library just as much as it needs a compiler. Up until now this was just an ambient assumption built-in to our infrastructure, but I'd prefer people to learn this at the same time as the necessity of
I really dislike this, because it would replace the nicely universal - {{ compiler('c') }}
- {{ stdlib('c') }}with a horrible mess of platform-specific selectors (or however the syntax for that ends up looking like in the new recipe format): - {{ compiler('c', stdlib='glibc') }} # [linux]
- {{ compiler('c', stdlib='macosx_deployment_target') }} # [osx]
- {{ compiler('c', stdlib='vs') }} # [win]
I agree that this is technically feasible, and with good output and documentation would be a workable solution. When I say I prefer to keep One reason that pushes this from a 60:40 (based on "less magic") to a 70:30 preference is that the C stdlib has a special role, in the sense that it can become necessary even without an explicit C compiler. In other words, we'd have to cover cases where a recipe with only |
|
|
||
| ## The `cdt` function | ||
|
|
||
| cdt stands for "core dependency tree" packages. These are repackaged from Centos. |
There was a problem hiding this comment.
Does "centos" have to be mentioned here? CDT packages could technically contain binaries from any Linux distribution right? I'm also wondering if we should add a link to some documentation that describe CDT more in details?
|
|
||
| Where `cdt_name` and `cdt_arch` are loaded from the variant config. If they are undefined, they default to: | ||
|
|
||
| - `cos6` for `cdt_name` on `x86_64` and `x86`, otherwise `cos7` |
There was a problem hiding this comment.
I feel a little bit uneasy with having default values hardcoded here. Do we really need to define default values in the spec? I feel like this kind of thing should be explicitly defined in an ecosystem (the central variant config in an ecosystem) instead of relying on some defaults.
There was a problem hiding this comment.
I agree that we should NOT have default values for CDTs. The choice of CentOS 6 and 7 was made by Anaconda and conda-forge long ago, but I don't think we should necessarily impose such choices on the ecosystem as a whole.
Further, imposing these defaults means we either have to indefinitely support CDTs from EOL Linux distributions (CentOS 6 hit EOL in 2020-Nov, and CentOS 7 will hit EOL in 2024-Jun), or the evaluation of ${{ cdt("...") }} changes depending on the version of {conda,rattler)-build you use. Neither of those outcomes seems ideal.
There was a problem hiding this comment.
Sure, we can get rid of this. I think we adopted this from conda-build where these values are also there by default.
There was a problem hiding this comment.
FWIW, in the new CDTs for Alma 8, we're planning to get rid of cdt_name.
There was a problem hiding this comment.
I'd also leave/move these out (see comment regarding defaults for compilers above).
There was a problem hiding this comment.
@h-vetinari - curious what that means exactly? A change to conda-build?
I'll remove the defaults from the CEP!
| - `cos6` for `cdt_name` on `x86_64` and `x86`, otherwise `cos7` | ||
| - To the `platform::arch` for `cdt_arch`, except for `x86` where it defaults to `i686`. | ||
|
|
||
| ## The `pin_subpackage` function |
There was a problem hiding this comment.
pin_subpackage is defined in another section. This section seems to only define the arguments of pin_subpackage and pin_compatible?
| host: | ||
| - numpy | ||
| run: | ||
| - ${{ pin_compatible('numpy', exact=True) }} |
There was a problem hiding this comment.
Does it support min_pin and max_pin? It's unclear right now in this spec.
There was a problem hiding this comment.
Yes, it does. Both use the same pin specification as mentioned earlier.
| version: "1.2.3" | ||
| requirements: | ||
| run: | ||
| - ${{ pin_subpackage('libfoo', exact=True) }} |
There was a problem hiding this comment.
Does it support min_pin and max_pin? It's unclear right now in this spec.
There was a problem hiding this comment.
They both are derived from the same "pin" function. So yeah, they both support the same inputs.
|
|
||
| ## The `cmp` function | ||
|
|
||
| The `cmp` function is used to compare two versions. It returns `True` if the comparison is true and `False` otherwise. |
There was a problem hiding this comment.
Should we add an example of how it can be used?
There was a problem hiding this comment.
Yes, what operators are available and what is the syntax?
There was a problem hiding this comment.
Ah so it's more like a version matcher. When I read compare I thought we were comparing two packages together like "is numpy's version greater than python's", which didn't make much sense to me.
But this is more like asking "Does the version of Python match this version expression?". So in that sense the name could be better expressed as:
version(python, "<3.8")version_match(python, "<3.8")match_version(python, "<3.8")satisfy(python, "<3.8")
Also this assumes that python will be represented by a concrete version x.y.z and not a spec like x.y.* right? IOW, it's already resolved to a specific package record.
If that's the case, should we specify that this function can only return after the solve has completed?
|
|
||
| You can also check for the existence of an environment variable: | ||
|
|
||
| - `${{ env.exists("MY_ENV_VAR") }}` will return `true` if the environment variable `MY_ENV_VAR` is set and `false` otherwise. |
There was a problem hiding this comment.
Nitpick. false and False are both used in the spec. It's hard to know if it returns a string or a bool.
There was a problem hiding this comment.
I added the word boolean true to make it clear that this is a bool value.
| The variant config could look something like: | ||
|
|
||
| ``` | ||
| c_compiler: | ||
| - gcc | ||
| c_compiler_version: | ||
| - "8.9" | ||
| cxx_compiler: | ||
| - clang | ||
| cxx_compiler_version: | ||
| - "12" | ||
| ``` |
There was a problem hiding this comment.
This seems a bit out of place; I think the example variant config should be provided before using its values to evaluate the function.
Additionally, we should provide an example variant config showing how to select different compilers for different platforms; e.g., are we keeping the conda_build_config.yaml mechanism?
c_compiler:
 - gcc # [linux]
- clang # [osx]
- vc # [win]
c_compiler_version:
 - "14.1" # [linux]
- "17.0" # [osx]
- "2022" # [win]There was a problem hiding this comment.
yeah we need to write a spec for the variant config (or even more global "config"). But IMO that shouldn't be part of this CEP.
|
|
||
| ## The compiler function | ||
|
|
||
| The compiler function is used to stick together a compiler from {lang}_compiler and {lang}_compiler_version |
There was a problem hiding this comment.
Which, if any, of {lang}_compiler and {lang}_compiler_version must be specified in the variant configuration? If either or both are optional, the CEP should describe what happens in the case when the user/recipe does not provide a value (i.e., are there default values?).
|
|
||
| ## The `cdt` function | ||
|
|
||
| cdt stands for "core dependency tree" packages. These are repackaged from Centos. |
|
|
||
| Where `cdt_name` and `cdt_arch` are loaded from the variant config. If they are undefined, they default to: | ||
|
|
||
| - `cos6` for `cdt_name` on `x86_64` and `x86`, otherwise `cos7` |
There was a problem hiding this comment.
I agree that we should NOT have default values for CDTs. The choice of CentOS 6 and 7 was made by Anaconda and conda-forge long ago, but I don't think we should necessarily impose such choices on the ecosystem as a whole.
Further, imposing these defaults means we either have to indefinitely support CDTs from EOL Linux distributions (CentOS 6 hit EOL in 2020-Nov, and CentOS 7 will hit EOL in 2024-Jun), or the evaluation of ${{ cdt("...") }} changes depending on the version of {conda,rattler)-build you use. Neither of those outcomes seems ideal.
| - `min_pin`: The lower bound of the dependency spec. This is expressed as a `x.x....` version where the `x` are filled in from the corresponding version of the package. For example, `x.x` would be `1.2` for a package version `1.2.3`. The resulting pin spec would look like `>=1.2` for the `min_pin` argument of `x.x` and a version of `1.2.3` for the package. | ||
| - `max_pin`: This defines the upper bound and follows the same `x.x` semantics but adds `+1` to the last segment. For example, `x.x` would be `1.(2 + 1)` for a package version `1.2.3`. The resulting pin spec would look like `<1.3` for the `max_pin` argument of `x.x` and a version of `1.2.3` for the package. |
There was a problem hiding this comment.
There seems a strong but unstated assumption here that version strings are integers separated by dots (.). If that is the case, that assumption should be explicitly stated and guidance provided as to what should happen if the assumption is violated. (Just sayin', because 1!24.4.0+91_gbfa8ccdce is a perfectly valid PEP-440/conda package version string.)
There was a problem hiding this comment.
Also, as written, it seems like min_pin and max_pin do not support an explicit version; e.g., recipes cannot simply have max_pin="1.2.3".
There was a problem hiding this comment.
Also, as written, it seems like
min_pinandmax_pindo not support an explicit version; e.g., recipes cannot simply havemax_pin="1.2.3".
Conda's existing pin_compatible jinja has both {min,max}_pin as well as {lower,upper}_bound, in order to be able to distinguish 'x.x' from an explicit version limit.
Perhaps it's easier to understand to overload both styles into two kwargs (one for lower, one for upper), and allow either 'x(.x)*' or an explicit version number? It's more work to implement, but less confusing IMO (otherwise it's easy to specify conflicting things e.g. by setting both max_pin and upper_bound)
There was a problem hiding this comment.
Wow, I wasn't aware! I found a few places where it's used but I don't know how it works. For example, in the onnx-feedstock it says:
- {{ pin_compatible('numpy', lower_bound='1.19', upper_bound='3.0') }} # [py>38]
Does this still take the lower bound from the build-time resolution? Or is this equivalent to just writing numpy >=1.19,<3.0?
There was a problem hiding this comment.
I also don't really understand why we would want this in the function. One can just as well write something like:
- numpy >=1.5
- ${{ pin_compatible("numpy", max_pin="x.x", min_pin="x.x.x") }}
If one needs additional (hard-coded) constraints. The solver doesn't really care about more constraints.
There was a problem hiding this comment.
@h-vetinari I'm on board with your proposal.
I suppose that could in theory create a problem if some upstream project decided to the string literals x.x, x.y, and x.z as their versions, but if that ever happens, I'm happy to say "NOPE" to attempting to package it. 😆
There was a problem hiding this comment.
@wolfv, given your changes in 0fda213, I presume you're not on board with this idea (or didn't see this thread)?
If you want to keep {min,max}_pin separate from {lower,upper}_bound, I think we need to enforce that at most one {min_pin,lower_bound} resp. {max_pin,upper_bound} each may be specified, everything else should raise an error.
I'd still prefer a unified interface, because I don't see the gain to keep these arguments separate (but rather requires a lot of complexity to ensure it is unambiguous/consistent)
There was a problem hiding this comment.
I indeed didn't see it or didn't read carefully enough. I also think this is a good change. The logic is already in place, so I don't think it will be a big change on the rattler-build side.
There was a problem hiding this comment.
OK, I just implemented that we only use lower_bound and upper_bound, but now I am wondering if we shouldn't have used min_pin and max_pin instead (as more people are using that these days). But this PR has the implementation: prefix-dev/rattler-build#918
There was a problem hiding this comment.
I proposed the API that I think is more natural and IMO more desirable long-term.
Obviously the naming choice is just bike shedding and both would work, but max_pin=1.2.3 seems off to me, whereas upper_bound='x.x' is fine because it's a more generic term, where users don't have to guess what a pin is or isn't.
|
|
||
| ## The `hash` variable | ||
|
|
||
| `${{ hash }}` is the variant hash and is useful in the build string computation. This used to be `PKG_HASH` in the old recipe format. Since the `hash` variable depends on the variant computation, it is only available in the `build.string` field and is computed after the entire variant computation is finished. |
There was a problem hiding this comment.
Should we explicitly say that this is a read-only variable? E.g., is providing "hash: deadbeef123455" in the variant config file explicitly prohibited?
There was a problem hiding this comment.
it's not really prohibited in the implementation right now. But we can say that it should be...
I don't have strong feelings about the name. We can change it to variant_hash if that's preferred!
|
|
||
| ## The `version_to_buildstring` function | ||
|
|
||
| - `${{ python | version_to_buildstring }}` converts a version from the variant to a build string (it removes the `.` character and takes only the first two elements of the version). |
There was a problem hiding this comment.
Should we provide an example here?
There was a problem hiding this comment.
This is the first time we introduce the | syntax as well. Can we do version_to_buildstring(python)?
There was a problem hiding this comment.
Yeah using a function would also work. But we also expose the default minijinja filters (such as lower, replace, ...), that can be used like ${{ "mystring" | upper }}
or ${{ val | replace('foo', 'moo') }}
There was a problem hiding this comment.
@jaimergp I added some more text regarding jinja filters to the bottom of the CEP.
| @@ -0,0 +1,125 @@ | |||
| # Jinja functions in recipes | |||
|
|
|||
| The new recipe format has some Jinja functionalities. We want to specify what functions exist and their expected behavior. | |||
There was a problem hiding this comment.
I feel this is lacking a short intro. At least we should link into the previous CEPs.
| The new recipe format has some Jinja functionalities. We want to specify what functions exist and their expected behavior. | |
| The new recipe format (introduced by CEP-XX, CEP-XY) has some Jinja functionalities. We want to specify what functions exist and their expected behavior. |
| - `${{ env.get("MY_ENV_VAR") }}` will return the value of the environment variable `MY_ENV_VAR` or throw an error if it is not set. | ||
| - `${{ env.get_default("MY_ENV_VAR", "default_value") }}` will return the value of the environment variable `MY_ENV_VAR` or `"default_value"` if it is not set. |
There was a problem hiding this comment.
Could this be overloaded to a single function? With the 2nd param being optional.
There was a problem hiding this comment.
yeah, that can work. We can also make it a named argument, e.g.
env.get("FOO", default="baz")
What do you think?
There was a problem hiding this comment.
@jaimergp I implemented this change: prefix-dev/rattler-build#917
Let me know what you think.
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Co-authored-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com>
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
|
@h-vetinari since we don't have a CEP for stdlib it might be good to define it here. What are the default values for the different operating systems? |
Sounds good!
I was under the impression that defaults here aren't necessarily a good thing? Perhaps even more than the compiler packages, the stdlib-metapackages don't have a "canonical" naming. Are you planning to set defaults for In any case, here are the specs that we're using in conda-forge. They won't change substantially anymore, barring unforeseen events. |
|
I expanded the content a bit. Some things that are left to decide:
|
| ```yaml | ||
| requirements: | ||
| host: | ||
| - ${{ "cudatoolkit" if cuda == "have_cuda" }} |
There was a problem hiding this comment.
Should we add an explicit callout for the case like this where expression can evaluate into "" or i guess null and that means this is excluded.
There was a problem hiding this comment.
We have something about that in line 500 - 510: https://github.com/conda/ceps/pull/71/files/c1028142a7f6e3e8a5d011667e0bec3b3f67ab3a#diff-0154c4169103373897853cfe5287bb7a1101289dd78c7fa30d2acba92b644938R500-R510
|
|
||
| ## Error handling | ||
|
|
||
| Build tools should be aggressive about Jinja errors: |
There was a problem hiding this comment.
Is "should" defined somewhere in the CEP repo? If not, an implementer could interpret it as optional compared to "must".
There was a problem hiding this comment.
Ideally we determine these in a provisional CEP that others use
|
@marcelotrevisani @jakirkham @mbargull @CJ-Wright last chance to vote - you can also choose |
|
NP on the title of this PR: Since this is I'm also not sure if it is worth codifying somewhere to prevent confusion. Maybe it is also worth it to add a section to the top of the CEP that specifies which |
|
For the record: voted "no" because while I like the ideas presented in this CEP, I don't think the CEP as written is ready to be adopted as a specification. IMO, various unanswered questions/comments need to be addressed before adoption (e.g., top-down evaluation of the context object). |
| - `target_platform` is a string that represents the platform for which the package is built. It is a string of the form `os-arch` (e.g. `linux-64`, `osx-64`, `win-64`, `linux-aarch64`, ...). | ||
| - `build_platform` is a string that represents the platform on which the package is built. Same format as `target_platform`. | ||
| - `linux`, `osx`, `win`, `emscripten`: These are boolean variables that are `true` if the target platform is a Linux, macOS, Unix, or Windows platform, respectively. Note that this is the first part of the `target_platform` string. | ||
| - `x86_64`, `aarch64`, `armv7l`, `ppc64le`, `s390x`, `sparc64`, `riscv64`: These are boolean variables that are `true` if the target platform is the respective architecture. Note that, except for `x86_64`, these are the second part of the `target_platform` string. |
There was a problem hiding this comment.
This does not capture the osx-arm64/win-arm64 cases.
Not a huge deal, but it would be good to mention here how those are handled.
|
|
||
| Context evaluation must happen from top-to-bottom. That means a later value can | ||
| reference an earlier one like so: | ||
|
|
||
| ```yaml | ||
| context: | ||
| version: "1.0.5" | ||
| name_and_version: "pkg_${{ version | replace('.', '_') }}" # evaluates to "pkg_1_0_5" | ||
| ``` |
There was a problem hiding this comment.
| Context evaluation must happen from top-to-bottom. That means a later value can | |
| reference an earlier one like so: | |
| ```yaml | |
| context: | |
| version: "1.0.5" | |
| name_and_version: "pkg_${{ version | replace('.', '_') }}" # evaluates to "pkg_1_0_5" | |
| ``` |
It would be better to leave this under-specified here instead of specifying it in conflict with the CEP-14.
This exact issue with the specification has been brought up by me in both previous CEPs for the new format and the wording here regresses on the issue instead of finally addressing it.
I do not understand why this simple issue is not being addressed, given that solutions have already been proposed.
jakirkham
left a comment
There was a problem hiding this comment.
Thanks Wolf! 🙏
Generally seems reasonable. Included some comments/suggestions. Otherwise would be good to add
| - `lower_bound`, defaults to `x.x.x.x.x.x`: the lower bound, either as a version | ||
| or as a "pin expression", or `None` | ||
| - `upper_bound`, defaults to `x`: the upper bound, either as a version or as a | ||
| "pin expression" or `None` |
| - The epoch is left untouched by the `max_pin` (or `min_pin`). If the epoch is | ||
| set, it will be included in the final version. E.g. `1!1.2.3` with a | ||
| `max_pin='x.x'` will result in `<1!1.3.0a0`. | ||
| - When bumping the version with a `max_pin` the local version part is removed. | ||
| For example, `1.2.3+local` with a `max_pin='x.x'` will result in `<1.3.0a0`. |
There was a problem hiding this comment.
Whereas this is specifying pins. Should we consolidate to one terminology or should we define both? Are there cases we need both?
| > `conda-build` uses the `lower_bound` for the version that is used in | ||
| > the `max_pin` pinning expression. `conda-build` also ignores the `min_pin` | ||
| > expression when a `upper_bound` is used. |
There was a problem hiding this comment.
Am curious about the motivation for the differences here
FWIW do find users confuse or forget about the options (usually the bounds). So consolidating somehow would makes sense
|
|
||
| You can also check for the existence of an environment variable: | ||
|
|
||
| - `${{ env.exists("MY_ENV_VAR") }}` will return a boolean `true` if the |
There was a problem hiding this comment.
get just returns None for undefined keys in Python and Jinja
|
|
||
| ## Error handling | ||
|
|
||
| Build tools should be aggressive about Jinja errors: |
There was a problem hiding this comment.
Ideally we determine these in a provisional CEP that others use
Co-authored-by: jakirkham <jakirkham@gmail.com>
|
The vote has been closed with the following result: Total voters: 15 (valid: 13 = 86.67%) Yes votes (11 / 84.62%):
No votes (1 / 7.69%)):
Abstain votes (1 / 7.69%):
Not voted (2):
Invalid votes (0): We reached quorum, and enough YES votes for this CEP to be accepted. 🎉 |
| The "old" recipe format has allowed arbitrary Jinja syntax (including set, | ||
| if/else or for loops). The new recipe format only allows a subset of Jinja with |
There was a problem hiding this comment.
| The "old" recipe format has allowed arbitrary Jinja syntax (including set, | |
| if/else or for loops). The new recipe format only allows a subset of Jinja with | |
| The "old" (`V0`) recipe format has allowed arbitrary Jinja syntax (including `set`, | |
| `if`/`else` or `for` loops). The "new" (`V1`) recipe format only allows a subset of Jinja with |
There was a problem hiding this comment.
We should probably also change old to V0 while we're at it.
There was a problem hiding this comment.
The title of this PR should be changed to reference V1 recipe format vs V2
| ## The `hash` variable | ||
|
|
||
| `${{ hash }}` is the variant hash and is useful in the build string computation. | ||
| This used to be `PKG_HASH` in the old recipe format. Since the `hash` variable |
There was a problem hiding this comment.
| This used to be `PKG_HASH` in the old recipe format. Since the `hash` variable | |
| This used to be `PKG_HASH` in the `V0` recipe format. Since the `hash` variable |
jinja functions in recipes
|
I don't know how we missed this but there we go, two years after approval, this CEP is hence minted as CEP 39. Merging! |
|
Thank you! |
No description provided.