Extra xtriggers

[ColemanTom]

I was wondering if you want a few extra xtriggers added to the core cylc code, or if the idea is to keep the number in cylc itself quite small and leave xtriggers to external users to implement locally only?

I'm making some for myself, and 3 of them I think might be suitable for general usage as they are not really specific to my use cases or environments.

https://github.com/ColemanTom/cylc/blob/7.8.x_extra_xtriggers/lib/cylc/xtriggers/file_exists.py
https://github.com/ColemanTom/cylc/blob/7.8.x_extra_xtriggers/lib/cylc/xtriggers/file_contains.py
https://github.com/ColemanTom/cylc/blob/7.8.x_extra_xtriggers/lib/cylc/xtriggers/remote_file_contains.py

[kinow]

Hi @ColemanTom

Once #2989 is merged, we will have the option to create a separate project like cylc-xtriggers (or cylc/xtriggers, etc).

For me this would be helpful as it would keep Cylc's core small and with less responsibilities. Users could request new features and get bugs fixed more quickly in this small project, without having to wait for the build and release of Cylc.

Furthermore, once we are using setuptools, we can make xtriggers as plugins, so if you had an xtrigger that was super specific for your environment, it could still be loaded automatically by Cylc's core, without the need to move it under certain directories, or append/prepend to sys.path. Making automation a bit easier.

Just my 0.02 cents, but still pending more discussion with @cylc/core

[sadielbartholomew]

To elaborate on your question @ColemanTom, it is my understanding (though @hjoliver knows best of course, having devised & implemented these) that xtriggers were designed to be a plugin interface, rather than as a means to extend our triggering mechanism capabilities per se.

The aim was to allow users to define any trigger that would be useful for them, leaving them to create them as they wish, hence we didn't plan to collect any such xtrigger custom function implementations as far as I am aware. Though we do classify the non-standard triggering systems we already supported before xtriggers were established, such as clock triggers & client-server triggers, as xtriggers, so the codebase includes those.

You have raised an important point in that there could be generic xtrigger functions that would feasibly be useful to a lot of the user base. Those you have created & linked seem like perfect examples. While it would be useful to keep these together in an open & advertised location, sadly I don't think we have the resources or time (given our roadmap & priorities) to be able to moderate these thoroughly.

That's why I really like @kinow's idea:

Once #2989 is merged, we will have the option to create a separate project like cylc-xtriggers (or cylc/xtriggers, etc). [...]

With a separate repository we can keep everything contained & have a disclaimer (or license or similar) that the repository is for user-created xtrigger functions that could be convenient but are not maintained or approved by @cylc/core. Perhaps trusted volunteers might want to help establish this repo, though, as having it under the official 'cylc organisation implies some accountability.

Furthermore, once we are using setuptools, we can make xtriggers as plugins [...]

This is also a great idea that I second. 💯 From my experience I know Sphinx (calling them 'extensions') & Vue make use of setuptools plugins t& they make extending the core systems very simple.

[ColemanTom]

Thanks for the feedback. So the idea would be to have a something/cylc-xtriggers repository on github in a different (forgive me, but I don't know the terminology) group (like rose is under metomi)?

Anyway, this all sounds quite reasonable and logical.

[kinow]

So the idea would be to have a something/cylc-xtriggers repository on github in a different group

It could be under the cylc organisation in GitHub I think. Something like cylc/cylc-xtriggers, or cylc/xtriggers. We have a few more repositories under the cylc organisation already (e.g. cylc-uiserver, cylc-web, cylc-admin).

Anyway, this all sounds quite reasonable and logical.

Thanks. We also have the challenge that with setup.py, Cylc 8 will be installed via pip, conda, and possibly rpm. With these tools, the Python code won't be somewhere like /opt/cylc. But more likely in some folder like $USER/anaconda3/lib/python3/site-packages/cylc, or /usr/lib/python3/site-packages/cylc.

The xtriggers shipped with Cylc will reside under this directory, but not all users will be able to add extra xtriggers there due to restrictions. So having this other repository, we could have shorter release cycles, and users would install it via pip install cylc-xtriggers, or conda install cylc-xtriggers.

Or if you created your own ColemanTom/test-xtriggers, following the contract for creating these pluggable xtriggers, you could install pip install . or pip install git+https://github.com/ColemanTom/test-xtriggers.git@master.

Of course we could also simply load it from other directories. So that's another possibility too.

[hjoliver]

@kinow -

The xtriggers shipped with Cylc will reside under this directory, but not all users will be able to add extra xtriggers there due to restrictions.

We do currently allow users to keep xtrigger functions in their own suite Python dirs.

However, I generally like your separate repository suggestion better anyway.

I can go ahead and create a new repository under the Cylc org for @ColemanTom's new functions. However, I'm still not clear on the naming convention, and how that will work under the new packaging methods: should it be cylc/cylc-xtriggers or cylc/xtriggers?

[kinow]

However, I'm still not clear on the naming convention, and how that will work under the new packaging methods: should it be cylc/cylc-xtriggers or cylc/xtriggers?

I have the same concern. We are going with cylc-_____ for now, but it looks repetitive. I think xtriggers is simpler, but we still have the issue with the module name (when installed via PYPI/Conda/etc). I guess we could have xtriggers, then use whatever directory structure we want, and in setup.py just set the module name to cylc-xtriggers or something like that.

[sadielbartholomew]

Just to chip in with a thought:

should it be cylc/cylc-xtriggers or cylc/xtriggers

If we anticipate that in the near or far future (in the latter case accounting for possible new directions, etc. we might not even entertain or imagine right now) there might be other cases, besides triggering, where it would be useful to provide entry points & allow users to create & use custom functionality, we could make the repository dedicated to more general "pluggable extensions" so that we can use the cool name cylc/cylcx i.e. Cylc X? X for extension &/or external (&/or extra, etc.)?

Admittedly, I'm not sure whether it is because I think this is an especially good idea, or simply because I really like Cylc X as a sub-project name...

[hjoliver]

@kinow - my reasoning for the current redundant cylc/cylc-blah naming convention was GitHub organisations are just a GitHub repository grouping thing, but the repository name cylc-blah has wider meaning (fork or clone cylc/cylc-blah and you end up with a repository cylc-blah). From what you say above, this doesn't necessarily determine the module name in setup.py (but does it in PYPI?) ... but maybe we should stick with cylc-blah for all cylc-related repository names to make sure the provenance is clear? (And if we do that, can we still arrange to use the code after installation, as from cylc import blah ... or not?). (Sorry, I'm still not sufficiently expert with Python packaging...).

[hjoliver]

@sadielbartholomew - not a bad idea and we can see what others think, but my inclination is to not group all current and future kinds of Cylc extension in a single repository because with proper packaging it seems easy to have multiple dependencies and then maintain them all separately (no need re-release all future "batch system" extensions (say) when one of our external triggers gets a bug fix, etc.).

[kinow]

From what you say above, this doesn't necessarily determine the module name in setup.py (but does it in PYPI?)

That's correct. It's more a convention that you normally find a project A with a structure A/A where A is its module (or A/src/A, or A/lib/python/A, etc). But you are actually free to have whatever structure you want. e.g. pyserial (installed via pip install pyserial) actually gives you the package serial.

but maybe we should stick with cylc-blah for all cylc-related repository names to make sure the provenance is clear?

That makes sense to me. But I am horrible naming and organizing things like this 😥

(And if we do that, can we still arrange to use the code after installation, as from cylc import blah ... or not?). (Sorry, I'm still not sufficiently expert with Python packaging...).

Yup, we have ways to configure the package name. The pull request to add setup.py uses cylc as package name (see here). So users will install it via pip install cylc (may need to change it if we go with a different name).

After installed, users actually have two modules added, cylc and parsec (though I expect the second to go away with #2880). This happens around this lines of the setup.py file.

[kinow]

with proper packaging it seems easy to have multiple dependencies and then maintain them all separately (no need re-release all future "batch system" extensions (say) when one of our external triggers gets a bug fix, etc.).

+1 🎉

[ColemanTom]

I would vote for cylc-xtriggers personally if I have a vote. When forking the repo out it will make it much clearer what it relates to. Just doing a quick google for github xtrigger gives a few different results.

[hjoliver]

Yep, same conclusion I came to above. Creating the new repo now...

[hjoliver]

@ColemanTom new repo here: https://github.com/cylc/cylc-xtriggers

Please post a PR there with your external trigger functions.

For the moment, they can probably just go in the top level (not a sub-dir) of the repository. We can move things around later once packaging is sorted out, if that has an impact.

I'm not sure about moving the existing xtriggers there at this stage (except for the Kafka example ... I'll move that) as they represent quite core functionality (clock triggers and other-suite triggers) and are documented in the User Guide. Do you have an opinion this @kinow ?

[hjoliver]

@ColemanTom - once the new PR is up, you can refer to this issue, then close this issue.

[kinow]

For the moment, they can probably just go in the top level (not a sub-dir) of the repository. We can move things around later once packaging is sorted out, if that has an impact.

+1

I'm not sure about moving the existing xtriggers there at this stage (except for the Kafka example ... I'll move that) as they represent quite core functionality (clock triggers and other-suite triggers) and are documented in the User Guide. Do you have an opinion this @kinow ?

I think these could be moved there too. We recently did a bit of re-work in the existing xtriggers. This could be released earlier before Cylc 7.8.2 or Cylc 8 were done. And later users could ask that other functions from cylc-xtriggers to be included in the core xtriggers... which could get a bit confusing to distinguish what should go in cylc or cylc-xtriggers.

After we get setup.py, and later provide a Setuptools' entrypoint for xtriggers, then we should be able to dynamically load xtriggers. Then we can move the code from cylc.xtriggers over to cylc/cylc-xtriggers, and users that rely on these xtriggers could:

• pip install cylc, to get just cylc, no xtriggers included
• pip install cylc-xtriggers, which automagically loads all xtriggers in the cylc/cylc-xtriggers repo 🎉

Another possibility, is to add a dependency group in cylc/cylc-flow's setup.py, something like [xtriggers], that would install cylc and cylc-xtriggers. And then add it to the [all] group. So users could cherry pick what they need for their environment, or just do pip install cylc[all] to get all dependencies, including xtriggers.

What do you think @hjoliver ?

[hjoliver]

I like 👍

[hjoliver]

For the moment, they can probably just go in the top level (not a sub-dir) of the repository. We can move things around later once packaging is sorted out, if that has an impact.

hmmm, actually maybe each xtrigger should be in its own directory, so that it can have accompanying documentation and/or usage examples.

[hjoliver]

OK, PRs posted to move the Kafka example xtrigger. I think we should move the others later, after #2990, so we can amend the user guide documentation at the same time, for how to get and use the xtriggers.

[ColemanTom]

Is moving the wall_clock one ideal? I find that one a bit awkward moving because the xtrigger_mgr code specifically looks for it, so its a bit of an odd case. All other xtriggers are stand alone. wall_clock sets a precedent to have modifications made to the core cylc code to handle xtriggers.

I'll start adding my currently made ones next week. Thanks all.

[kinow]

From what I remember without looking at the code now, it was a special Xtrigger, that did not seem to be designed to be changed or extended by users.

So perhaps we could instead leave it here for now and decide what to do about it later.

[hjoliver]

Yes, clock-triggering is core functionality. Although it is only singled out in the main library because it is executed synchronously in the main loop (because a clock check is known to be quick and doesn't need the full asynchronous treatment). (So now it occurs to me we could move it too, to the xtriggers repo, if we allowed the xtrigger writer to specify synchronous or asynchronous operation).

[kinow]

(So now it occurs to me we could move it too, to the xtriggers repo, if we allowed the xtrigger writer to specify synchronous or asynchronous operation)

Interesting! Now that we have the setup.py in Cylc repository I will prepare a pull request to cylc-flow & to cylc-xtriggers in the next days to support the entry points, and prepare the directory structure the new cylc-xtriggers module.

The next step would then be to move everything to their final destination 👍

[ColemanTom]

Although it is only singled out in the main library because it is executed synchronously in the main loop

@hjoliver I don't believe that is technically accurate. wall_clock has an argument passed to it by the core library, whereas other xtriggers don't.

But, I had a proposal on that point. I was thinking that xtriggers could benefit from a small adjustment. If all xtriggers had to have a signature which included **kwargs, e.g. foo(a=a, b=b, **kwargs), then, cylc itself could make all calls to the xtrigger include a dictionary. Said dictionary would include point_as_seconds which the wall_clock xtrigger uses, but also all of the currently available %(x)s items as defined in https://cylc.github.io/doc/built-sphinx-single/index.html#custom-trigger-functions. This would mean arguments like the suite name, owner, debug mode, cycle point, run dir, share dir, and so on, are available to any xtrigger automatically through the kwargs dictionary, and a suite user would then not have to define them. It also means the uniqueness of wall_clock is reduced.

[hjoliver]

@kinow -

Now that we have the setup.py in Cylc repository I will prepare a pull request to cylc-flow & to cylc-xtriggers in the next days to support the entry points, and prepare the directory structure the new cylc-xtriggers module.

Thanks, that would be really good.

[hjoliver]

@ColemanTom -

@hjoliver I don't believe that is technically accurate. wall_clock has an argument passed to it by the core library, whereas other xtriggers don't.

You mean point_as_seconds? I'm sure we could remove that difference of handling.

All I really meant was: the fundamental reason for the different, simpler, treatment of clock xtriggers was that they can be called synchronously ... whereas arbitrary user-supplied trigger functions can't be guaranteed to be sufficiently well behaved for that. After making that distinction, i guess I built on it slightly!

[hjoliver]

@ColemanTom - regarding your proposal above, do you mean essentially what we're already doing for the general (non-clock) case? If so, then i think I agree that would be the way to "remove that difference of handling" as per my previous comment.

[ColemanTom]

I'm going to close this. I don't think it really matters that much.