Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recipe CEP III for all Jinja functionality #71

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

wolfv
Copy link
Contributor

@wolfv wolfv commented Apr 12, 2024

No description provided.

@h-vetinari
Copy link

In conda/conda-build#5053 you had said:

I do think that for the future (ie. rattler-build) we'll have an upcoming CEP to define this feature better.

Is this that CEP? In any case, I don't see {{ stdlib(...) }} in the diff, and I think it needs to be covered in some way, given that we're now rolling this out at scale.

@wolfv
Copy link
Contributor Author

wolfv commented Apr 13, 2024

Yep, this is the one. Do you want to have it as a separate function or rolled into compiler? I prefer to roll it into compiler...

@h-vetinari
Copy link

h-vetinari commented Apr 13, 2024

Yep, this is the one. Do you want to have it as a separate function or rolled into compiler? I prefer to roll it into compiler...

I'd prefer a separate function, because it's less magic to have to understand (i.e. "why do I need to specify c_stdlib_version and how does that affect my build environment?" is much easier to answer if there is a {{ stdlib("c") }} under build:); also, the coupling of compiler and stdlib versions is entirely incidental (at least for C), so I'd like to not conflate them conceptually.

@wolfv
Copy link
Contributor Author

wolfv commented Apr 14, 2024

My points:

  • There are already two variables for compilers (c_compiler and c_compiler_version). Adding additional 2 variables (that are non-mandatory) is fine IMO, especially if it's well-documented.
  • It complicates things for beginners. I think recipes would continue to work without adding the stdlib (correct?). So it's something they should add but don't have to add. For compiler it's clear - the recipe will not work without a compiler.
  • In most cases, it will always be added when there is a compiler.

I can imagine two behaviors:

  • We control the stdlib behavior from the variant_config. This means, if the variant config contains entries for c_stdlib / c_stdlib_version we use them from the compiler function.
  • We add additional arguments to the jinja function, so that compiler('c', stdlib='glibc').

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 stdlib comes from (we already point out that the clangxx_osx-arm64 package comes from the compiler, for example).

And lastly, I just think it's a matter of writing really good documentation.

@h-vetinari
Copy link

h-vetinari commented Apr 14, 2024

I think recipes would continue to work without adding the stdlib (correct?).

Define "work". When we increase the c_stdlib_version on linux and macos later this year, the artefacts produced by a recipe without {{ stdlib("c") }} would continue to work on modern systems, but would be broken on systems with a too-old C standard library, because it's through the meta-packages pulled in by stdlib that we set the correct run-exports in the artefact metadata.

It would only affect very old systems, but I'd still consider that broken.

So it's something they should add but don't have to add. For compiler it's clear - the recipe will not work without a compiler.

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 {{ compiler(...) }} - the two go hand in hand (as implied by having the same "c" argument, and the same way the keys are constructed), even though the relevant versions are completely independent from each other.

I can imagine two behaviors:

  • We add additional arguments to the jinja function, so that compiler('c', stdlib='glibc').

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]
  • We control the stdlib behavior from the variant_config. This means, if the variant config contains entries for c_stdlib / c_stdlib_version we use them from the compiler function.

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 {{ stdlib("c") }} explicit, this is not an all-or-nothing statement, but more a 70:30 kind of preference over the implicit handling through the compiler.

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 {{ compiler("cxx") }} or {{ compiler("rust") }} also includes c_stdlib{,_version}.

cep-recipe-jinja.md Outdated Show resolved Hide resolved

## The `cdt` function

cdt stands for "core dependency tree" packages. These are repackaged from Centos.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the above.


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`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we can get rid of this. I think we adopted this from conda-build where these values are also there by default.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, in the new CDTs for Alma 8, we're planning to get rid of cdt_name.

- `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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it support min_pin and max_pin? It's unclear right now in this spec.

version: "1.2.3"
requirements:
run:
- ${{ pin_subpackage('libfoo', exact=True) }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it support min_pin and max_pin? It's unclear right now in this spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add an example of how it can be used?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, what operators are available and what is the syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some examples!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick. false and False are both used in the spec. It's hard to know if it returns a string or a bool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the word boolean true to make it clear that this is a bool value.

cep-recipe-jinja.md Outdated Show resolved Hide resolved
cep-recipe-jinja.md Outdated Show resolved Hide resolved
Comment on lines 25 to 36
The variant config could look something like:

```
c_compiler:
- gcc
c_compiler_version:
- "8.9"
cxx_compiler:
- clang
cxx_compiler_version:
- "12"
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the above.


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`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +99 to +101
requirements:
run:
- ${{ pin_subpackage('libfoo', exact=True) }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an indentation error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks OK to me? :)

Comment on lines 57 to 58
- `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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

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)

Copy link
Contributor Author

@wolfv wolfv May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

(https://github.com/conda-forge/onnx-feedstock/blob/49b7f95ea64d2b0e174f23ac62fe0f59441491d7/recipe/meta.yaml#L49)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't really understand why we would want this in the function.

I wanted this for numpy2, but that's a rare enough case that it probably doesn't matter. In that case, we're building against 2.0, but I want the lower bound to be 1.19, which cannot be done with what you propose (because it's looser than what pin_compatible enforces).

More generally though, in your example, I find that adding additional constraints just feels like a weaksauce work-around. If I'm already using a function to specify the constraint on numpy, why won't it give me the expressivity needed to do that?

Does this still take the lower bound from the build-time resolution? Or is this equivalent to just writing numpy >=1.19,<3.0?

I think the intention of the author was clear (be compatible with numpy 2 on py>38), but it doesn't work because it just creates an additional constraint, where - again - it would need to be looser. IOW, onnx's metadata contains both:

numpy >=1.19,<3.0
numpy >=1.26.4,<2.0a0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that's a rare enough case that it probably doesn't matter

On second thought, probably it does. That onnx recipe is precisely trying to work around the same thing that I need to handle in the numpy run-export - in other words, building against numpy >=1.25 (with no extra manual overrides) gives a run-time lower bound of >= 1.19 currently.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it doesn't work because it just creates an additional constraint, where - again - it would need to be looser.

Ah wait, maybe it actually does work correctly, and the package just falls prey to the numpy run-export, which adds the numpy >=1.26.4,<2.0a0 on top.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wolfv @chenghlee WDYT about the idea of having pin-or-version arguments in pin_compatible? As an example, let's say for numpy 2.0.1:

pin_compatible("numpy")                                         --> numpy >=2.0.1,<3    # default: lower_bound='x.x.x.x.x.x', upper_bound='x'
pin_compatible("numpy", upper_bound="x.x")                      --> numpy >=2.0.1,<2.1
pin_compatible("numpy", lower_bound="x.x", upper_bound="2.3")   --> numpy >=2.0,<2.3
pin_compatible("numpy", lower_bound="1.19")                     --> numpy >=1.19,<3     # most libs compiled against np2
pin_compatible("numpy", lower_bound="1.19", upper_bound="2.3")  --> numpy >=1.19,<2.3   # scipy's usecase

The API contract could be that anything matching x(\.x)* is a pinning expression, and the rest gets treated as an explicit version.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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. 😆


## 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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we explicitly say that this is a read-only variable? E.g., is providing "hash: deadbeef123455" in the variant config file explicitly prohibited?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we call it variant_hash?


## 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).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we provide an example here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first time we introduce the | syntax as well. Can we do version_to_buildstring(python)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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') }}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this is lacking a short intro. At least we should link into the previous CEPs.

Suggested change
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.

cep-recipe-jinja.md Outdated Show resolved Hide resolved
${{ compiler('c') }}
```

This would pull in the c_compiler and c_compiler_version from the variant config. The compiler function also adds the target_platform to render to something such as:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This would pull in the c_compiler and c_compiler_version from the variant config. The compiler function also adds the target_platform to render to something such as:
This would pull in the `c_compiler` and `c_compiler_version` values from the variant config. The compiler function also adds the `target_platform` value to render to something such as:


The variant config could look something like:

```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
```
```yaml


The function thus evaluates to `{compiler}_{target_platform} {compiler_version}`.

The variant config could look something like:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure "could" is the right verb here. Are we suggesting? Standardizing? If it's not set in stone, why is it useful in this doc? As Cheng proposes, we should standardize the configuration file here.

cep-recipe-jinja.md Outdated Show resolved Hide resolved
cep-recipe-jinja.md Outdated Show resolved Hide resolved
cep-recipe-jinja.md Outdated Show resolved Hide resolved
- ${{ pin_subpackage('libfoo', exact=True) }}
```

## The `cmp` function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## The `cmp` function
## The `cmp` function

I prefer a more explicit name like compare_versions.

Comment on lines 120 to 121
- `${{ 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be overloaded to a single function? With the 2nd param being optional.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing a section on error handling:

  • What should happen when Jinja functions fail to process the input?
  • Should they render to null or just the input?
  • Should they raise the error?

Not sure if this should be a general section or a subsection in each function.

wolfv and others added 5 commits May 10, 2024 09:31
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>

The new recipe format has some Jinja functionalities. We want to specify what functions exist and their expected behavior.

## The compiler function

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the discussion on stdlib further upthread died down, just leaving a note that this either needs to add the stdlib function, or propose an alternative way of how to realize the same functionality (and how it gets populated through the variant config).

@wolfv
Copy link
Contributor Author

wolfv commented May 20, 2024

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

@h-vetinari
Copy link

since we don't have a CEP for stdlib it might be good to define it here.

Sounds good!

What are the default values for the different operating systems?

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 {{ compiler(...) }} as well? Looking at the diff here it doesn't seem like there are default being proposed?

In any case, here are the specs that we're using in conda-forge. They won't change substantially anymore, barring unforeseen events.

c_stdlib:
  - sysroot                    # [linux]
  - macosx_deployment_target   # [osx]
  - vs                         # [win]
m2w64_c_stdlib:                # [win]
  - m2w64-toolchain            # [win]              <-- might change to m2w64-sysroot in the future
c_stdlib_version:              # [unix]
  - 2.17                       # [linux]            <-- per July 2024
  - 10.13                      # [osx and x86_64]
  - 11.0                       # [osx and arm64]

@wolfv
Copy link
Contributor Author

wolfv commented May 21, 2024

I expanded the content a bit.

Some things that are left to decide:

  • Limit the filters from Jinja to a default subset?
  • Should we add a split(str) filter (useful for version numbers, so that one can do "${{ version | split(".") | slice(0, 2) | join(".") }} or something along those lines. We could also create a more specialized function/filter for version string manipulation of this sort.
  • Should env work more like Python env
    • env["FOO"] instead of env.get("FOO")
    • env.get("FOO", "default") instead of env.get_default("FOO", "default")
  • Or should it be more like Github Actions
    • env.FOO instead of env.get("FOO")
    • env.FOO or "default" for default values
  • Should ${{ compiler('c') }} evaluate directly to gcc_linux-64 or should there be an intermediate representation (as is now)
  • For pin_subpackage it will be impossible to evaluate it directly (due to self-referential nature of the function).
  • Should we add lower_bound and upper_bound to the pin functions? And how should they work (in conjunction with min_pin and max_pin)?
  • The final name for the cmp function.

@schuylermartin45
Copy link

schuylermartin45 commented May 21, 2024

  • Limit the filters from Jinja to a default subset?

  • Should we add a split(str) filter (useful for version numbers, so that one can do "${{ version | split(".") | slice(0, 2) | join(".") }} or something along those lines. We could also create a more specialized function/filter for version string manipulation of this sort.

  • Should env work more like Python env

    • env["FOO"] instead of env.get("FOO")
    • env.get("FOO", "default") instead of env.get_default("FOO", "default")
  • Or should it be more like Github Actions

    • env.FOO instead of env.get("FOO")
    • env.FOO or "default" for default values

I can't speak as much for the needs of package builders, but I will say I think any allowed JINJA syntax needs to minimized. It adds a significant level of complication in the development of automated tooling (namely package editing). But if we must have them, we should limit the syntax and make the rules of using the features very clear. The more variety available, the more developers will use the tools in "clever" ways, the more breakages we will have when trying to automatically edit recipes.

Comment on lines +129 to +130
> c: vs2017
> cxx: vs2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nicer to use unversioned vs here? This is a relatively recent addition, but that way the version would be independent of the default name, like on unix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are inheriting the standards from conda-build here. I am happy to change them, and using an non-versioned vs would probably be better (altough also potentially annoying when it moves since you have to re-install vs on the build machine).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And lastly, not sure what Anaconda has available on the defaults channel ...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

altough also potentially annoying when it moves since you have to re-install vs on the build machine

Generally, our vs packages in conda-forge will pick up the compilers that you have - we have a fallback to use the local version (& toolchain target) if our default activation fails. Also, I'm not saying to abolish the 2017 / 2019 / 2022 pins, just move them to c{,xx}_compiler_version, like for unix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Didn't know that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @jezdez @chenghlee what is the situation on Anaconda's channels here? Should we make vs the default?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid defaults. I think default values in tools are counter intuitive and should be defined on a per ecosystem basis and not by tools. In the sense that conda-forge should define their default compilers and Anaconda should define their default compilers, same for all the other ecosystems.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify my thoughts: Defaults like this are (or can) help less experienced package builders, but it can also be very disturbing and annoying for more experienced package builders. It can also cause troubles for ecosystems that might want to opt-out of these defaults (for example because they want to be very explicit about what they support and how things should work).

I sympathize to the cause of making it easier to use the build tools, but there are things which are very ecosystem specific. I believe compilers is one of these things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way around this is that we make an endpoint in the repodata or so where we can easily download the conda_build_config.yaml for a specified channel to get these defaults per-channel.

If we don't have any easy way it means that each user needs at least two files per recipe which is already a bit annoying, no?


## The `stdlib` function

The `stdlib` function works exactly as the `compiler` function.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if that's the intention here, but perhaps a bit of clarification nevertheless.

Suggested change
The `stdlib` function works exactly as the `compiler` function.
The `stdlib` function works exactly as the `compiler` function, except that it has no pre-defined defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this section is not complete yet. I think we should define defaults for stdlib? We need to make the experience of conda recipes better...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this section is not complete yet.

Ah, that wasn't clear to me. I'm ambivalent about the usefulness of defaults, but if your goal is that people are able to write recipes outside of cf (where we have the defaults in the global pinning anyway) & Anaconda main channels and not have to worry about where their compiler is coming from, then I guess that's a usecase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would like to have an awesome "default" experience. If you take a recipe from conda-forge, ideally you can just run build on it without having to worry about the variant config file.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would like to have an awesome "default" experience. If you take a recipe from conda-forge, ideally you can just run build on it without having to worry about the variant config file.

The thing is that this is very ecosystem specific. The default might work for some time if conda-forge uses the same defaults, but as soon as an ecosystem like conda-forge diverges from the CEP defaults, then the recipes might not be buildable without a variant config. Same for the defaults channel.

cep-recipe-jinja.md Outdated Show resolved Hide resolved
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Comment on lines 173 to 174
- `min_pin=x.x, max_pin=x.x` would result in `>=1.21,<1.22`
- `min_pin=x.x.x, max_pin=x` would result in `>=1.21.3,<2`
Copy link
Contributor

@jaimergp jaimergp May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the resolution of max_pin add the .0dev0 component too to avoid the dev versions leaking in in the version constraint?

Current behavior in solvers is that <2 actually includes things like v2.0b1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what conda-build does?

I think the pre-release discussion is another one (maybe?). I would really like to have some behavior like on PyPI where you can opt-in to pre-releases. But I think that's for another CEP.

Just for reference, I didn't try to invent something new with these pin expressions but rather just copy whatever conda-build does.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be a good idea from the POV of least surprise. It happened in the numpy 2 migration that people were surprised that 2.0.0rc2 actually satisfies <2 (myself included).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Care to write a CEP?! :) Would love to introduce that behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what conda-build does?

Apparently it does handle this case. See gau2grid recipe + its rendered run_exports including the a0 bit.

The code in conda-build has it here. That should better be a 0dev0, though, since that's technically the lowest bound (dev < alpha).


## The `cdt` function

CDT stands for "core dependency tree" packages. These are repackaged from Centos.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment disappeared, so re-adding it:

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?


A pin has the following arguments:

- `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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `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.
- The package name
- `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.

?


The new recipe format allows for inline conditionals with Jinja. If they are falsey, and no `else` branch exists, they will render to an empty string (which is, for example in a list or dictionary, equivalent to a YAML `null`).

When we render the recipe, we filter all values that are `null` from the YAML output. This allows us to write and parse the following easily:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of saying "we". should it say "the build tool must"? I've seen "we" used in other places in this CEP too. ALso, same question for "us".


If `cuda` is not equal to yes, the first item of the host requirements will be empty (null) and thus filtered from the final list.

This also works for dictionary keys:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This also works for dictionary keys:
This also works for dictionary values:

- Or should it be more like Github Actions
- `env.FOO` instead of `env.get("FOO")`
- `env.FOO or "default"` for default values
- Should `${{ compiler('c') }}` evaluate directly to `gcc_linux-64` or should there be an intermediate representation (as is now)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this an implementation detail of the tool? In the sense that even if you have an intermediate representation, the end result will be gcc_linux-64 11.1.0 right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants