Skip to content
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

Support EmPy version 4 #6248

Open
wants to merge 3 commits into
base: 8.3.x
Choose a base branch
from
Open

Support EmPy version 4 #6248

wants to merge 3 commits into from

Conversation

hjoliver
Copy link
Member

A forum post has revealed that EmPy 3 doesn't play well with scheduler daemonization:

https://cylc.discourse.group/t/empy-import-shell-environment-variables/985/1

If I simply add #!EmPy to a flow.cylc, the workflow runs fine but:

  • I get a traceback at start-up
  • stdout does not end up in the log
  • stdout still appears in the terminal

Something to do with EmPy replacing sys.stdout

--- Logging error ---
Traceback (most recent call last):
  File "/home/oliverh/cylc/cylc-flow/cylc/flow/loggingutil.py", line 167, in emit
    self.do_rollover()
  File "/home/oliverh/cylc/cylc-flow/cylc/flow/loggingutil.py", line 234, in do_rollover
    os.dup2(self.stream.fileno(), sys.stdout.fileno())
AttributeError: 'ProxyFile' object has no attribute 'fileno'

The good news: EmPy 4.1 works fine, but it requires a small change to our code.

Anyone aware of any reason not to require EmPy > 4 ?

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.

@hjoliver hjoliver added this to the 8.3.x milestone Jul 19, 2024
@hjoliver hjoliver self-assigned this Jul 19, 2024
@hjoliver hjoliver changed the base branch from master to 8.3.x July 19, 2024 01:38
@oliver-sanders
Copy link
Member

From the looks of the EmPy changelog, there are quite a few breaking changes at version 4.

However, as far as I'm aware, there are no users of EmPy in Cylc workflows at all. The team who added this support have subsequently moved to Jinja2. Note, their motivation in adding EmPy was stability (i.e. nothing changes in EmPy) at a time when Jinja2 was somewhat volatile. So, TBH we would be quite happy to drop EmPy entirely. Either way, I don't think there's an issue with requiring EmPy 4. Perhaps put a post out on Discourse to see if anyone it using it that we are not aware of (probably not given the bug)?

@hjoliver
Copy link
Member Author

However, as far as I'm aware, there are no users of EmPy in Cylc workflows at all.

Well there's the user who posted the bug to the forum!

@oliver-sanders
Copy link
Member

oliver-sanders commented Jul 19, 2024

That's a new workflow, so no!

@hjoliver
Copy link
Member Author

The team who added this support have subsequently moved to Jinja2.

Interesting, why did they move to Jinja2?

Note, their motivation in adding EmPy was stability (i.e. nothing changes in EmPy) at a time when Jinja2 was somewhat volatile.

Didn't they like the more Python-like nature of EmPy too? Stability seems a somewhat crazy reason given that you don't have to go with new versions of a package until some compatibity issue arises, and that no package still in use never changes (hence EmPy 4 as we've just discovered!).

So, TBH we would be quite happy to drop EmPy entirely. Either way, I don't think there's an issue with requiring EmPy 4. Perhaps put a post out on Discourse to see if anyone it using it that we are not aware of (probably not given the bug)?

👍

@oliver-sanders
Copy link
Member

oliver-sanders commented Jul 19, 2024

Stability seems a somewhat crazy reason given that you don't have to go with new versions of a package until some compatibity issue arises

This was back in the period when Jinja2 was making frequent syntax changes in subsequent releases. Most notably, the highly controversial scoping changes (which bound scope to loops making it impossible to e.g. build a list in a loop) which were introduced one version before namespaces() (an object which is permitted to persist beyond the loop scope) were introduced creating an incompatibility island.

Keeping up with these changes was a challenge and a portability nightmare, this sort of issue ultimately led us to bundle Jinja2 with Cylc so that Jinja2 version changes could be synchronised with Cylc version changes.

never changes (hence EmPy 4 as we've just discovered!).

Take a look at the empy release history, it's extremely slow. Some projects are faster moving than others, empy is easier for template developers to keep up with than Jinja2 because it barely changes. Note, it's a one person project with a closed repository.

@cgrudz
Copy link

cgrudz commented Jul 19, 2024

However, as far as I'm aware, there are no users of EmPy in Cylc workflows at all.

Well there's the user who posted the bug to the forum!

Hey there, I'm just chiming in on the discussion, I posted the bug report yesterday. I'm new to Cylc so I've been evaluating the different ways that I can use its supported features / ecosystem, and I've been testing out its integration with these templating packages. My use-case is that I'm converting a number of legacy Bash/Python Rocoto-driven NWP workflows and job-array-driven NWP verification workflows to become a more robust, feature rich and sustainable suite of Cylc-driven workflows for case study analysis and for an eventual integration with our center's near-real-time forecast systems. I've been testing both of Jinja2 and EmPy, and I think my team could use either one, but without having a lot of existing Jinja2 codes, and as many of my younger colleagues are already proficient with Python, we'd get more value out of templating in Python syntax and idioms. It's not a huge obstacle for any of us to adopt Jinja2, but I think many new Cylc users will fall into a similar use-case as ours, as NCL is increasingly replaced with Python and Python usage is becoming increasingly widespread for geophysical data science. If it's not a huge burden to support EmPy or a similar package, I can definitely see the value.

@oliver-sanders
Copy link
Member

Hi,

Sorry for the delayed reply (been a bit busy lately).

The context here is that several years back (2018) someone contributed EmPy support to Cylc, but their project subsequently moved back to Jinja2 (?) and no one returned to the Cylc project to maintain EmPy support. The EmPy v4 release (Nov 2023) appears to have broken Cylc compatibility, but this went unnoticed for nearly a year because there aren't presently any Cylc projects making use of EmPy support.

Supporting other template engines is perfectly reasonable, but maintaining support is a bit of effort if no one uses it. Additionally I'm a little wary of EmPy because:

  • The project appears to be the work of a single person.
  • The code repository is not public, or at least sufficiently well buried that I can't find it (you can send the maintainer an email, but you can't send them a pull request).
  • The project has previously gone quiet for years at a time (there was an 11 year gap between 3.3 and 3.3.1).
  • The conda-forge feedstock appears to be unmaintained (the last Conda release is 3.3.4 which is 5 years old).

Which sadly makes it a high risk dependency. If something breaks in EmPy, you may get a fix (I hear the maintainer is very helpful), but one day they won't be maintaining it any more, in the absence of a succession plan, this will leave EmPy users stranded. The closed nature of the project may also be considered a security risk.

So, if it was two months ago (before you had spotted EmPy support in Cylc), I would have voted to remove support (before anyone actually tried to use it!).

If you're keen and accept the caveats above, perhaps we could consider giving it another go.

@cgrudz
Copy link

cgrudz commented Aug 20, 2024

@oliver-sanders, thanks for the detailed reply and your analysis of this dependency, sorry for my own slow response. I think that you raised a lot of important issues about the long-term support for templating with EmPy and I appreciate that this could be a very dangerous road to go down if there isn't greater transparency about the EmPy project's future. As far as my team is concerned, we can switch fully to using Jinja2 for templating if that receives first-tier support with Cylc, we don't have too much invested in this yet and both syntaxes are fairly similar to work with.

Cheers!
Colin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants