Xtrigger function arg validation.

[hjoliver]

Close #5448

Targeting master not 8.1.x because this conflicts with #5291

This is not just a bug-fix for the wall_clock xtrigger - I've gone for a generic approach:

• if an xtrigger module has a function called validate_config it will be called at validation time, with the parsed args.

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).
• CHANGES.md 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.

[wxtim]

I'm picking this up to add some tests.

@hjoliver - It says it is targeting master, but the PR currently targets 8.2.x - You say it conflicts with #5291, but that's against master, and has now been merged.

[hjoliver]

@hjoliver - It says it is targeting master, but the PR currently targets 8.2.x - You say it conflicts with #5291, but that's against master, and has now been merged.

Yes, that was outdated info, I've crossed it out.

Rebased onto latest 8.2.x

[hjoliver]

This now has a bunch of off-topic (but trivial) typing tweaks - flake8 was complaining in my environment.

[MetRonnie]

This conflicts/will conflict with #5831 which has been merged into master

I created a temporary branch off master and merged in this branch - https://github.com/MetRonnie/cylc-flow/tree/xtrigger-tmp

However, I get an error when using clock_1 = wall_clock() xtrigger which does not seem related to #5831

ERROR - wall_clock function kwargs needs trigger time or offset
    Traceback (most recent call last):
      File "~/cylc-flow/cylc/flow/scheduler.py", line 743, in start
        await self.configure(params)
      File "~/cylc-flow/cylc/flow/scheduler.py", line 498, in configure
        self._load_pool_from_point()
      File "~/cylc-flow/cylc/flow/scheduler.py", line 784, in _load_pool_from_point
        self.pool.load_from_point()
      File "~/cylc-flow/cylc/flow/task_pool.py", line 188, in load_from_point
        self.spawn_to_rh_limit(tdef, point, {flow_num})
      File "~/cylc-flow/cylc/flow/task_pool.py", line 730, in spawn_to_rh_limit
        self.add_to_pool(ntask)
      File "~/cylc-flow/cylc/flow/task_pool.py", line 239, in add_to_pool
        self.create_data_store_elements(itask)
      File "~/cylc-flow/cylc/flow/task_pool.py", line 250, in create_data_store_elements
        self.data_store_mgr.increment_graph_window(
      File "~/cylc-flow/cylc/flow/data_store_mgr.py", line 791, in increment_graph_window
        self.generate_ghost_task(
      File "~/cylc-flow/cylc/flow/data_store_mgr.py", line 1264, in generate_ghost_task
        self._process_internal_task_proxy(itask, tproxy)
      File "~/cylc-flow/cylc/flow/data_store_mgr.py", line 1515, in _process_internal_task_proxy
        sig = self.schd.xtrigger_mgr.get_xtrig_ctx(
      File "~/cylc-flow/cylc/flow/xtrigger_mgr.py", line 411, in get_xtrig_ctx
        raise ValueError(
    ValueError: wall_clock function kwargs needs trigger time or offset
CRITICAL - Workflow shutting down - wall_clock function kwargs needs trigger time or offset

May be worth changing milestone to master and sorting out the problem there?

[hjoliver]

May be worth changing milestone to master and sorting out the problem there?

Done.

(This branch originally targeted master, not sure why we re-directed at some point)

[hjoliver]

Redrafting this briefly - I've had to redo the arg validation bit for entry points. Done, but just checking the changes locally before pushing them up.

[hjoliver]

This changes the cylc.xtrigger entry point from function to module.

If the loaded module contains a validate() function (as well as the xtrigger function), that will be called to check the xtrigger args parsed from the config.

[wxtim]

I'm not convinced that this is safe - I think that it needs to be if not elif. I've had a go at writing integration tests for these, and I was going to write some unit tests too, but I've run out of time.

[hjoliver]

It's valid because of line 50. i.e. it has to be one or the other because the config file can only have (e.g.) wall_clock(PT1H) or wall_clock(offset=PT1H).

[wxtim]

Yes, that makes sense - I was projecting backward from the xrandom test where I started out with a copy of this.

[hjoliver]

Note on the new validate(f_args, f_kwargs) functions (esp. for @wxtim, who's writing some):

We decided to allow positional and keyword args in xtrigger config. E.g. these are equivalent:

• wall_clock(PT1H)
• wall_clock(offset=PT1H)

Unfortunately this can make argument validation tricky (you may have to check both args and kwargs for a given item).

[wxtim]

I've made an additional check which I think is worth mentioning - I've scraped our local workflow run-dirs for 'wall_clock\((.*)\)' and run all of these args/kwargs against the wall clock trigger to validate, to ensure existing workflows aren't broken by this change. Interestingly, we only have 52 patterns in total.

[wxtim]

Replaced by #5955