-
Notifications
You must be signed in to change notification settings - Fork 83
fix: special case jinja #1612
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
base: main
Are you sure you want to change the base?
fix: special case jinja #1612
Conversation
We can change this further to allow Jinja expressions to return We could then also make the following use case work, where a user expands a list into YAML:
|
Using this recipe:
we still cannot get the 12 as string, i am still working on it to check if we can make a solution
on powershell |
Indeed, it seems to go wrong somewhere in the code. context:
cuda_version: ${{ env.get("RAPIDS_CUDA_VERSION") }}
cuda_major: ${{ (env.get("RAPIDS_CUDA_VERSION") | split("."))[0] }}
cuda_major_is_string: ${{ (env.get("RAPIDS_CUDA_VERSION") | split("."))[0] is string }} Running Gives us: [
{
"recipe": {
"schema_version": 1,
"context": {
"cuda_version": "12.8.0",
"cuda_major": 12,
"cuda_major_is_string": true
},
"package": {
"name": "test-package",
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a strong -1 on this. Jinja is fundamentally a string templating engine. It is being used to build yaml by manipulating strings. So if you want the example in this post to come out as a string, you need to include quotes in the yaml itself via
"${{ '1234' }}"
Special casing jinja2 to behave in odd ways that force types to cross into the yaml is going to lead to odd breakages/cases, and breaks the mental model of what jinja2 is doing.
@wolfv There is a huge issue that is not producable in WSL and windows, but it is in all github runners, which is mainly about the unit tests (pkg_hash is the most serious one) I tried to solve it, change the formatters for hash, change the calculation logic and none of them passes. It also doesn't recognize
now, which is another test fails now. Tried to fix that as well but the only solution i could find for 5 hours :D was making it
|
@beckermr the problem that we are fixing is the following:
This is quite surprising I find. Having to wrap Jinja expressions in quotes to force a string is more cumbersome IMO. The code in this PR works quite well and is less surprising when dealing with env vars and similar things. |
Yes. Jinja always does string substitutions. The types of items do not cross the boundary of the ${{ }} expression. The issue with having the types cross is that for people who use jinja properly as a string templating, it becomes ambiguous and confusing. This case is now really hard to predict:
Does the types cross the boundary of the jinja quote the internal quotes this pr implicitly adds? Does it ignore them? Does it turn them to single quotes? Does it render a nonsense yaml value? Why is one of these obviously the answer when the others are not? This PR actually makes the interaction of the jinja templating with the yaml LESS clear. The rule for jinja2 is simple: "jinja produces strings that are inserted into the yaml." Anything else is more complicated and surprising as you work through the edge cases. |
For this case:
The (outer) quotes will always win and force whatever is inside to be a string. The more ambigous case (but not really, due to outer quotes) would be:
where the inner type is an integer but the quotes cast it to a string. |
Yep. Those examples exactly illustrate my point. I have written out explicitly how the jinja+yaml works for four cases below. a
renders using the following steps
b
renders using these steps
c
renders using these steps
d
renders using the following steps
This PR would change the behavior of case d to insert a quoted string instead of the value of the string (so step 3 changes) based on the type of the underlying jinja2 expression (computed in case 1). However, if that rule is applied consistently, then case b should yeild The only way to tell b should be different than d in an automated way is to figure out what type YAML would parse and then account for that. That extra logic where the jinja2 parser has to reach out beyond itself to figure out why type to return breaks the clean separation between the jinja2 parsing and the yaml parsing. The downstream consequence is that we've deviated from the standard behavior for these tools meaning that
This entire thing reminds me of selectors in conda build v0 recipes. Instead of using the tools as intended, extra syntax and meaning was introduced by using python |
I think I can live with your point @beckermr. However, one thing that doesn't work right now is:
And I think we should make that work (this will be currently interpreted as The other thing that will be much harder to make work is:
We could potentially serialize the file as JSON on a single line and insert it that way into the YAML to get something that we could later index into. But not sure if that is a great idea. One workaround would be something like:
But we don't advertise this capability widely :) |
Unfortunately these special cases are also outside standard jinja2 and yaml behavior. Thus I don't think you can change their behavior without the same consequences as above. |
There is a confusing behavior about types with rattler-build and minijinja.
When we have something like:
Then in the currently released version of rattler-build,
1234
will become an integer because the Jinja evaluates to1234
which will be interpreted by YAML as a integer.However, we can fix this by special casing Jinja expressions (starting with
${{
and ending with}}
). We can fix this by evaluating the Jinja as an expression and look at the Jinja type that comes out.If it is a string, we can then quote the string and forbid YAML to coerce this type, so that a string stays a string.
This will also fix this situation:
Where we may run into trouble is with something like:
This would be evaluated as integer
1234
with the current code.However, we could also decide to allow booleans or numbers to only come from simple Jinja expressions and not proper "templates" (ie. always cast anything more complex than a single expression to string).