7.9.x Update to jinja2 2.11.1 #3502

kinow · 2020-02-16T23:30:46Z

These changes close #2572

Sibling PR of #3493

Requirements check-list

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Already covered by existing tests.
Appropriate change log entry included.
No documentation update required.

kinow · 2020-02-16T23:34:06Z

tests/validate/43-jinja2-template-error-main.t

@@ -33,7 +33,7 @@ __SUITERC__
 run_fail "${TEST_NAME_BASE}" cylc validate 'suite.rc'
 contains_ok "${TEST_NAME_BASE}.stderr" <<'__ERROR__'
 Jinja2Error:
-    'You can only sort by either "key" or "value"'
+    raise FilterArgumentError('You can only sort by either "key" or "value"')


The other changes are the result of copy-and-paste of Jinja2 2.11.1 ZIP contents, minus their asyncio files as that breaks the build due to Py3 code.

After copying the files, this was the only test that failed. Debugging it, it looks like the traceback contents changed in 2.11.1, and instead of having just the text on a new line, it contains the raise FilterArgumentError part too.

Wondering if that's safe to update... I don't think users rely on this?

Seems fine to me.

(What were the asyncio files for though? Is it surprising that deleting those didn't entirely break the new Jinja2?)

Curious, 2.11.1 claims to have 2.7 support. What aspect of the build was breaking? Was is our tests (e.g. pycodestyle) or something functional?

kinow · 2020-02-16T23:44:33Z

Compared the output of the new Jinja2 library side-by-side with 7.8.x and master PR branches, and the traceback changed a bit.

For the 7.8.x one test failed as instead of starting a new line with 'You can only...', it starts now with raise FilterArgumentError('You can only...').

hjoliver · 2020-02-17T02:56:38Z

@kinow - well done getting this to work with 2.7

@oliver-sanders - this fixes a known bug, but are you particularly concerned about Jinja2 back-compat for Met Office Cylc-7 users? (Given that we bundle Jinja2 with Cylc 7, I'm not sure this statement is as true for Cylc 7 as it is for Cylc 8...)

oliver-sanders · 2020-02-17T03:39:13Z

this fixes a known bug, but are you particularly concerned about Jinja2 back-compat for Met Office Cylc-7 users?

Jinja2 bugs in workflows are nasty to debug, worse still as workflows often have highly-branched Jinja2 code bugs go unnoticed until a later date by which time the relation to the change of Cylc version is no longer obvious.

So Jinja2 changes need to be well managed and documented to give users a fighting chance. Sneaking a Jinja2 upgrade into a maintenance release would be unhelpful. It would break the trust of our release schedule if maintenance releases have significant interface changes.

From a change management perspective 8.0.0 is a more natural home for this change. But as 8.0.0 is so far off we could bump 7.8.5 to 7.9 and make a (small) minor release out of this.

oliver-sanders · 2020-02-17T03:44:20Z

Second thoughts, from the MO perspective we might still require Python2.6 compatibility for internal release which this would break. Will check.

If so we would want to push to this to 7.9 and maintain a 7.8.x branch for any future bugfixes.

hjoliver · 2020-02-17T03:45:26Z

From a change management perspective 8.0.0 is a more natural home for this change. But as 8.0.0 is so far off we could bump 7.8.5 to 7.9 and make a (small) minor release out of this.

Can do. Is that OK with you @kinow (so your work here won't be wasted 👍 ) (For background, there are many well-known and well-used Cylc-7 suite configs that are essentially massive Jinja2 "programs" 😬 )

hjoliver · 2020-02-17T03:52:21Z

(OK awaiting comment from @dpmatthews ... from offline chat with @oliver-sanders we could put this PR to 7.9.0, and maintain 7.8.x as well for the few remaining bug fixes...)

kinow · 2020-02-17T04:30:38Z

Happy if this is postponed to 7.9 or targeted only for 8.0. Whichever works best.

dpmatthews · 2020-02-17T13:29:42Z

I don't think we can remove Python 2.6 compatibility in a 7.8.x release so this would need to become 7.9. We still have Cylc in use on RHEL 6 systems so would have to stick to 7.8. Unless there is demand from others, I suggest we make this an 8.0 change.

TomekTrzeciak · 2020-02-17T15:52:11Z

I don't think we can remove Python 2.6 compatibility in a 7.8.x release so this would need to become 7.9. We still have Cylc in use on RHEL 6 systems so would have to stick to 7.8. Unless there is demand from others, I suggest we make this an 8.0 change.

@dpmatthews, legacy RHEL 6 machines are not the only ones that use python 2.6 for running cylc on our site. Just saying...

hjoliver · 2020-02-17T18:51:06Z

Unless there is demand from others, I suggest we make this an 8.0 change.

Then all users for the next year (ish) will have to live with Jinja2 "incorrect context" bug, which is moderately serious (wrong line numbers are often worse than none, when debugging).

So I think putting to 7.9 would be better, for those not stuck on Python 2.6. We don't expect many more changes on 7.x so maintaining two branches for subsequent fixes will be easy enough.

dpmatthews · 2020-02-17T20:52:12Z

So I think putting to 7.9 would be better, for those not stuck on Python 2.6. We don't expect many more changes on 7.x so maintaining two branches for subsequent fixes will be easy enough.

Fair enough. I assume this pull request will need to be changed to point to a new 7.9.x branch. The installation guide will also need an update to reflect the Python 2.7 requirement.

CHANGES.md

oliver-sanders · 2020-02-17T21:00:19Z

tests/validate/43-jinja2-template-error-main.t

@@ -33,7 +33,7 @@ __SUITERC__
 run_fail "${TEST_NAME_BASE}" cylc validate 'suite.rc'
 contains_ok "${TEST_NAME_BASE}.stderr" <<'__ERROR__'
 Jinja2Error:
-    'You can only sort by either "key" or "value"'
+    raise FilterArgumentError('You can only sort by either "key" or "value"')


Curious, 2.11.1 claims to have 2.7 support. What aspect of the build was breaking? Was is our tests (e.g. pycodestyle) or something functional?

kinow · 2020-02-17T21:06:10Z

@oliver-sanders

Curious, 2.11.1 claims to have 2.7 support. What aspect of the build was breaking? Was is our tests (e.g. pycodestyle) or something functional?

Just this functional test. The output of their traceback changed a bit, so it wasn't matching the output correctly.

oliver-sanders · 2020-02-17T22:19:02Z

Just this functional test. The output of their traceback changed a bit, so it wasn't matching the output correctly.

Sorry the context of that comment got skewed. I was referring to the removal of the async files.

kinow · 2020-02-17T22:28:57Z

Just this functional test. The output of their traceback changed a bit, so it wasn't matching the output correctly.

Sorry the context of that comment got skewed. I was referring to the removal of the async files.

Ah, that one. Travis still has the logs: https://travis-ci.org/kinow/cylc-flow/jobs/645797353

I think it was due to some functional or unit test loading that file?

hjoliver · 2020-02-18T23:10:58Z

@kinow I've created the new 7.9.x branch, so you can re-target this PR.

kinow · 2020-02-18T23:28:52Z

Updated PR title
Rebased against 7.9.x (thanks @hjoliver !) with GitHub UI
Updated changelog (solving a request from @oliver-sanders to update it, thanks!)
Push-forced, so now Travis should be busy for a while running functional tests

hjoliver · 2020-02-18T23:32:01Z

I'll merge this if it passes the functional tests again (it should) ... we've all agreed in principle, and it doesn't change any files outside of lib/jinja2/ ...

hjoliver

Tests pass 👍 ; this changes lib/jinja2/ only.

kinow and others added 2 commits February 4, 2020 19:02

Update to Jinja2 2.11.1

7197802

Update test to match what Jinja2 2.11.1 outputs

b9b995d

kinow changed the title ~~Update to jinja2 11 1~~ [cylc-flow 7.8.x] Update to jinja2 2.11.1 Feb 16, 2020

kinow commented Feb 16, 2020

View reviewed changes

kinow force-pushed the update-to-jinja2-11-1 branch from d197dc9 to 4bed434 Compare February 16, 2020 23:39

kinow changed the title ~~[cylc-flow 7.8.x] Update to jinja2 2.11.1~~ 7.8.x Update to jinja2 2.11.1 Feb 16, 2020

kinow self-assigned this Feb 16, 2020

kinow added this to the cylc-7.8.5 milestone Feb 16, 2020

kinow requested review from hjoliver and oliver-sanders February 16, 2020 23:45

oliver-sanders modified the milestones: cylc-7.8.5, cylc-7.9.0 Feb 17, 2020

oliver-sanders requested changes Feb 17, 2020

View reviewed changes

kinow changed the base branch from 7.8.x to 7.9.x February 18, 2020 23:21

kinow changed the title ~~7.8.x Update to jinja2 2.11.1~~ 7.9.x Update to jinja2 2.11.1 Feb 18, 2020

Add changelog

a0b5678

kinow force-pushed the update-to-jinja2-11-1 branch from 4bed434 to a0b5678 Compare February 18, 2020 23:27

hjoliver approved these changes Feb 19, 2020

View reviewed changes

hjoliver merged commit 520abe1 into cylc:7.9.x Feb 19, 2020

oliver-sanders mentioned this pull request Mar 27, 2020

Jinja error in validation shows incorrect context #2572

Closed

oliver-sanders mentioned this pull request Jul 15, 2021

Jinja2 errors - line context failure #2708

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7.9.x Update to jinja2 2.11.1 #3502

7.9.x Update to jinja2 2.11.1 #3502

kinow commented Feb 16, 2020 •

edited

Loading

kinow Feb 16, 2020

hjoliver Feb 17, 2020

hjoliver Feb 17, 2020

oliver-sanders Feb 17, 2020

kinow commented Feb 16, 2020

hjoliver commented Feb 17, 2020

oliver-sanders commented Feb 17, 2020 •

edited

Loading

oliver-sanders commented Feb 17, 2020 •

edited

Loading

hjoliver commented Feb 17, 2020

hjoliver commented Feb 17, 2020

kinow commented Feb 17, 2020

dpmatthews commented Feb 17, 2020

TomekTrzeciak commented Feb 17, 2020

hjoliver commented Feb 17, 2020

dpmatthews commented Feb 17, 2020

oliver-sanders Feb 17, 2020

kinow commented Feb 17, 2020

oliver-sanders commented Feb 17, 2020

kinow commented Feb 17, 2020

hjoliver commented Feb 18, 2020

kinow commented Feb 18, 2020

hjoliver commented Feb 18, 2020

hjoliver left a comment

7.9.x Update to jinja2 2.11.1 #3502

7.9.x Update to jinja2 2.11.1 #3502

Conversation

kinow commented Feb 16, 2020 • edited Loading

kinow Feb 16, 2020

Choose a reason for hiding this comment

hjoliver Feb 17, 2020

Choose a reason for hiding this comment

hjoliver Feb 17, 2020

Choose a reason for hiding this comment

oliver-sanders Feb 17, 2020

Choose a reason for hiding this comment

kinow commented Feb 16, 2020

hjoliver commented Feb 17, 2020

oliver-sanders commented Feb 17, 2020 • edited Loading

oliver-sanders commented Feb 17, 2020 • edited Loading

hjoliver commented Feb 17, 2020

hjoliver commented Feb 17, 2020

kinow commented Feb 17, 2020

dpmatthews commented Feb 17, 2020

TomekTrzeciak commented Feb 17, 2020

hjoliver commented Feb 17, 2020

dpmatthews commented Feb 17, 2020

oliver-sanders Feb 17, 2020

Choose a reason for hiding this comment

kinow commented Feb 17, 2020

oliver-sanders commented Feb 17, 2020

kinow commented Feb 17, 2020

hjoliver commented Feb 18, 2020

kinow commented Feb 18, 2020

hjoliver commented Feb 18, 2020

hjoliver left a comment

Choose a reason for hiding this comment

kinow commented Feb 16, 2020 •

edited

Loading

oliver-sanders commented Feb 17, 2020 •

edited

Loading

oliver-sanders commented Feb 17, 2020 •

edited

Loading