-
Notifications
You must be signed in to change notification settings - Fork 94
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
make conda recipe data-loading stricter #1349
Conversation
Moving this back to draft, there is a failure I need to investigate. Sorry for the noise. |
{% for e in data.get("project", {}).get("scripts", {}).items() %} | ||
- {{ e|join(" = ") }} | ||
{% for entrypoint in data["project"]["scripts"] %} | ||
- {{ entrypoint ~ ' = ' ~ data["project"]["scripts"][entrypoint] }} |
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 know this one looks a little weird. I can explain.
Switching from this
{% for e in data.get("project", {}).get("scripts", {}).items() %}
to this
{% for e in data["project"]["scripts"].items() %}
results in an error like this:
File "/__w/dask-cuda/dask-cuda/conda/recipes/dask-cuda/meta.yaml", line 24, in top-level template code
{% for e in data["project"]["scripts"].items() %}
TypeError: 'NoneType' object is not callable
It seems that calling dictionary methods like .items()
or .iteritems()
isn't supported... maybe conda-build
overrides .get()
or __getattr__
or getattr()
, or maybe that's something Jinja2
is doing.
BUT... this pattern where you for
-iterate over keys, and then use those to subset the list, totally works 😁
And it does come with a bit more safety. I tried removing the [project.scripts]
table in pyproject.toml
completely. On branch-24.08
, that results in the recipe silently building "successfully" without those entrypoints defined.
On this branch, it results in an informative error.
Error: Failed to render jinja template in /Users/jlamb/repos/dask-cuda/conda/recipes/dask-cuda/meta.yaml:
'dict object' has no attribute 'scripts'
See "How I tested this" in the PR description for details on how I tested this.
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 can explain, I promise!" is always a good thing to write on a diff. 😉 I think this is totally fine.
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.
haha yeah, thanks 😂
{% for e in data.get("project", {}).get("scripts", {}).items() %} | ||
- {{ e|join(" = ") }} | ||
{% for entrypoint in data["project"]["scripts"] %} | ||
- {{ entrypoint ~ ' = ' ~ data["project"]["scripts"][entrypoint] }} |
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 can explain, I promise!" is always a good thing to write on a diff. 😉 I think this is totally fine.
/merge |
Contributes to rapidsai/build-planning#72
Proposes using
[]
subsetting instead of.get()
in templating statements in the conda recipe that read data out ofpyproject.toml
. That'll ensure that we get a big loud build error if changes topyproject.toml
remove some sections that the conda recipe expects to exist.Notes for Reviewers
How I tested this
Rendered the recipe.
It looks correct to me (click for details)