Reload custom Jinja2 extensions

[Sanjays2402]

Closes #7385

Custom workflow Jinja2 modules remained cached across reload, so edited filters, tests, and globals kept their old behavior. This reloads matching local modules when rebuilding the Jinja environment and adds a regression test.

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).
• Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
• Tests are included (or explain why tests are not needed).
• Changelog entry included if this is a change that can affect users
• Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
• If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

[oliver-sanders]

Hi, thanks for your interest in this issue.

Out of interest, why did you pick the reload interface used here, which alternatives were considered?

We haven't fully narrowed the scope of the issue yet so there's a bit of an investigation component to this, there are likely to be scenarios other than the one example explicitly given in #7385. For example, there can be Python files in a workflow other than Jinja2 extensions and imports can also lie outside of a workflow.

This fix will need to hash out all of the possible scenarios where changes could be made to Python files between workflow being started and reloaded, and how Cylc will behave in each scenario which we should then back up with tests.

[Sanjays2402]

Good questions, and fair — I jumped to an implementation before the scope was settled. To answer directly: I picked the Jinja2-extension reload path because #7385's example was specifically about a custom Jinja2 extension not being picked up on reload, so I hooked the reload where the Jinja2 environment is rebuilt. It was the narrowest change that fixed the reported case, not a considered take on the broader problem.

You're right that it doesn't generalize. The cases I hadn't accounted for:

• Plain Python modules imported by the workflow (not Jinja2 extensions) — these are cached in sys.modules and my change does nothing for them.
• Imports resolving outside the workflow directory (site-packages, PYTHONPATH entries) — reloading those is a different and riskier proposition, and probably shouldn't be done implicitly at all.
• The question of what "reload" even means for a Python file whose top-level code already ran with side effects.

So this PR as-is only covers one branch of a bigger decision tree. I'm happy to either (a) narrow it explicitly to "Jinja2 extensions only" with that scope documented, and leave the broader Python-reload question to a separate design discussion, or (b) hold it while the scope gets hashed out if you'd rather solve it whole. What's your preference?

[hjoliver]

It might still be worth merging this as-is on the basis that is a small straightforward change and users would naturally expect that a modified and reinstalled Jinja2 filter should be picked up on reload - so this is still an improvement toward the more general goal, and the implementation here will be very easy to modify in future if a more general solution is found. What do you think @oliver-sanders ?

[oliver-sanders]

This might not be far off of the full solution, it just needs a little thought...

Before we review/merge, we should work out the list of scenarios to which bug #7385 applies. For example:

• (from the OP): A Jinja2 filter is changed, then the workflow is reloaded.
• A Python file in the workflow's lib/python directory is changed and a Jinja2 global is importing this; etc).

The desired behaviour in all scenarios, is that any changes made to Python files should take effect when the workflow is reinstalled & reloaded (see the cylc vr).

We may or may not be able to address all scenarios, but we need to work out what they are so we can document this fix as well as any outstanding caveats which we don't plan to address. We could also do with tests for these.

@Sanjays2402, would you be able to help with this?

---

For reference, see:

• https://cylc.github.io/cylc-doc/stable/html/user-guide/writing-workflows/configuration.html#workflow-configuration-directories
• https://cylc.github.io/cylc-doc/stable/html/user-guide/writing-workflows/jinja2.html

[Sanjays2402]

On the interface choice: I went with the reload path because that's where the stale module is actually observable, and it keeps the change local rather than touching install. I didn't seriously evaluate invalidating at install time, which is probably the other candidate given cylc vr does both.

Agree on scoping it properly first. The scenarios I can see are: (1) a Jinja2 filter/extension changed then reloaded, (2) a module under the workflow's lib/python changed while a Jinja2 global imports it, (3) an import resolving outside the workflow entirely, which I don't think we can sanely invalidate and is probably a documented caveat rather than a fix, and (4) a Python file changed without a reinstall, where I'd expect no change to take effect.

Happy to write those up as tests. Worth agreeing the expected behaviour for (3) before I do, since that's the one where "correct" is a judgement call rather than obvious.

[oliver-sanders]

(3) an import resolving outside the workflow entirely, which I don't think we can sanely invalidate and is probably a documented caveat rather than a fix

I suspect we cannot reasonably "cascade" the reload down to imports of imports, etc, so it is completely reasonable for this to be a documented caveat.

The main concern from the Cylc perspective is that files directly managed by Cylc (i.e, by install/reinstall) adhere to the lifecycle of Cylc operations (e.g, config, play, reload).

[Sanjays2402]

Agreed on (3) — cascading down through imports of imports isn't something we can do safely, so a documented caveat it is.

On the "files directly managed by Cylc" point, I went and checked what the current patch actually covers, and it's narrower than I'd claimed. Scenario matrix as it stands:

1. Jinja2Filters/Jinja2Globals/Jinja2Tests file changed, then reinstall + reload — fixed by this PR. The module is looked up in sys.modules, and reloaded if its __file__ is the same file we're about to load, so a reinstalled filter takes effect.
2. A module under the workflow's lib/python changed, imported by one of those Jinja2 files — not fixed. I ran this: a global gl.py doing import helper where helper.val() returns 'old', then rewrote helper.py to return 'new' and rebuilt the environment. Still 'old'. lib/python gets appended to sys.path in fileparse.py and whatever is imported from it stays in sys.modules untouched — importlib.reload() on the top-level extension doesn't re-execute its imports.
3. An import resolving outside the workflow (site-packages, CYLC_PYTHONPATH) — caveat, don't touch.
4. Python file changed with no reinstall — no effect, which I think is the correct behaviour since the installed run directory hasn't changed.

So (2) is the gap, and it's squarely in "files Cylc manages". The tractable version is to invalidate sys.modules entries whose __file__ lives under the workflow's lib/python before rebuilding the Jinja2 environment — one level deep by path ownership rather than by dependency graph, which stays on the right side of the (3) line since anything outside the run directory is left alone. That does mean a module holding top-level state gets re-executed, which is a behaviour change worth calling out in the docs.

Two things I'd like your call on before I write it:

• Is path-scoped invalidation (lib/python only) the shape you want, or would you rather keep this PR at (1) and open the lib/python case as a separate issue?
• Should invalidation happen at reload, or at reinstall time given cylc vr does both? I put it at reload because that's where the staleness is observable, but install is arguably the more honest hook.

Either way I'll add functional tests covering (1) and (2) plus a doc note for (3) and (4).