Jinja2: catch and log warnings - #7365
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves observability of Jinja2 preprocessing by capturing Python warnings raised during templating and emitting them via Cylc’s logger, helping users see deprecations (e.g. Jinja2/MarkupSafe changes) that were previously silent.
Changes:
- Capture warnings during
jinja2process(...)inread_and_procand log them after preprocessing. - Modernize/clarify typing annotations (move away from
typing as t, use built-in generics and|unions).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
cylc/flow/parsec/fileparse.py |
Adds warnings capture around Jinja2 preprocessing and logs captured warnings; updates typing/imports. |
cylc/flow/parsec/jinja2support.py |
Updates typing annotations/import formatting (no behavioral change). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
oliver-sanders
left a comment
There was a problem hiding this comment.
👍
Asserted that the built-in filters do not raise warnings:
#!jinja2
# {{ 'P1D' | duration_as('s') }}
# {{ 1 | pad(5) }}
# {{ '10661004T08+01' | strftime('%H') }}| "The following warnings were raised during Jinja2 " | ||
| "preprocessing:\n" |
There was a problem hiding this comment.
Should tailor this message to make it clearer that this is a call to action on the part of the user.
There was a problem hiding this comment.
How about?
| "The following warnings were raised during Jinja2 " | |
| "preprocessing:\n" | |
| "The following warnings were raised during Jinja2 " | |
| "preprocessing (if not addressed, your workflow may break " | |
| "in future versions of Cylc):\n" |
or
| "The following warnings were raised during Jinja2 " | |
| "preprocessing:\n" | |
| "The following warnings were raised during Jinja2 " | |
| "preprocessing (please address at the earliest " | |
| "opportunity):\n" |
There was a problem hiding this comment.
It's a hard one to phrase. We really want to say, "this will break at Cylc 8.7.0", but that might not be true.
There was a problem hiding this comment.
Perhaps something like:
Any Jinja2 v3.1 deprecations will break at Cylc 8.7.0
There was a problem hiding this comment.
If you are putting in version specific text anyway, would it be appropriate to detect 'v3.1' or something like that in the warning and alter the text to be more specific?
oliver-sanders
left a comment
There was a problem hiding this comment.
That aught to do it.
Can remove the codicil at 8.7.0 - #7368
|
(Tutorial tests failing for unrelated HTTP 404, all other tests passing. Merging with 2 approvals and only minor test fix in last force push) |
* Back support Jinja2 interfaces deprecated at 3.0 and removed at 3.1. * Jinja2 renamed a bunch of functions and deprecated their old names. * Our users didn't spot this as Python warnings are not visible to them. * As of cylc#7365 (8.6.6) Python warnings originating from Jinja2 will be turned into Cylc warnings. * To provide a migration window, the old function names will be supported in Cylc 8.7.x and removed in 8.8.0.
* Back support Jinja2 interfaces deprecated at 3.0 and removed at 3.1. * Jinja2 renamed a bunch of functions and deprecated their old names. * Our users didn't spot this as Python warnings are not visible to them. * As of cylc#7365 (8.6.6) Python warnings originating from Jinja2 will be turned into Cylc warnings. * To provide a migration window, the old function names will be supported in Cylc 8.7.x and removed in 8.8.0.
* Back support Jinja2 interfaces deprecated at 3.0 and removed at 3.1. * Jinja2 renamed a bunch of functions and deprecated their old names. * Our users didn't spot this as Python warnings are not visible to them. * As of cylc#7365 (8.6.6) Python warnings originating from Jinja2 will be turned into Cylc warnings. * To provide a migration window, the old function names will be supported in Cylc 8.7.x and removed in 8.8.0.
* Back support Jinja2 interfaces deprecated at 3.0 and removed at 3.1. * Jinja2 renamed a bunch of functions and deprecated their old names. * Our users didn't spot this as Python warnings are not visible to them. * As of cylc#7365 (8.6.6) Python warnings originating from Jinja2 will be turned into Cylc warnings. * To provide a migration window, the old function names will be supported in Cylc 8.7.x and removed in 8.8.0.
Follow-on from #7325 (comment)
Partly addresses #6850
Example
Whereas before, no warning would be seen.
Note
It seems warnings that are already emitted by the time the config file is processed do not get re-emitted, so the current
sre_constantswarning from isodatetime will not be logged (even if the user importssre_constantsthemselves. Not sure how to get around this so I'm just going to leave it as a mixed blessing, unless anyone has any ideas)Check List
CONTRIBUTING.mdand added my name as a Code Contributor.?.?.xbranch.