Add a new entry_point for xtriggers #5831

ColemanTom · 2023-11-21T00:08:21Z

This creates a clean way to add xtriggers from python packages. xtriggers no longer need to be directly in the PYTHONPATH but can instead be from a general installation location using an entry_point cylc.xtriggers. Similar could be done for other things perhaps, like Jinja2Globals, although I have not looked.

I do not have an Issue for this, but I think it may be or at least relate to #3456, although I do not add the built in xtriggers with this PR, perhaps they should be but I think that can be a bit of work for someone else.

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 Add documentation for xtriggers entry_point cylc-doc#671

I've not added tests as I've not looked at how it coudl be done (can it cleanly?). I'll update CHANGES and cylc doc only if/when this is agreed to.

Example of use below, but note that with this approach, you do not actually need to have a file per xtrigger with a function in it with the same name as the filename.

def xtriggers():
    return {
        'xtrig1': xtrig1.xtrig1,
        'xtrig2': xtrig2.xtrig2,
        'abc': abc.abc,
    }

Then in pyproject.toml

[project.entry-points."cylc.xtriggers"]
my_package = "my_package.cylc_configure:xtriggers"

ColemanTom · 2023-11-21T00:55:49Z

If this approach is agreed to and you want it into 8.2.X I can backport it to that instead.

oliver-sanders

Happy with using entry points for xtriggers.

We started using entry points with Cylc 8 and have been slowly propagating them.

Features should target the next minor release (8.3.0) so this shouldn't be backported.

cylc/flow/scripts/install.py

cylc/flow/subprocpool.py

oliver-sanders · 2023-11-21T10:22:16Z

Note, the commit seems to be associated with a different GitHub account, this is ok, but please make sure the .mailmap file maps both addresses against your name, if this is done correctly the git shortlog -se command should only list your name once. (it's legal stuff to do with the CLA)

oliver-sanders · 2023-11-21T10:26:42Z

From #3456:

To close this issue (the Cylc Flow side of things)

Implement the cylc.plugins.xtriggers entry point.

Register the built-in Xtriggers via this mechanism.

ColemanTom · 2023-11-21T22:56:05Z

Register the built-in Xtriggers via this mechanism.

I've added a new file xtrigger_entry_point.py, but contemplated adding the entry_point into xtrigger_mgr.py. Happy to do whatever you want here.

ColemanTom · 2023-11-21T23:02:58Z

Note, the commit seems to be associated with a different GitHub account, this is ok, but please make sure the .mailmap file maps both addresses against your name, if this is done correctly the git shortlog -se command should only list your name once. (it's legal stuff to do with the CLA)

Things appear to map correctly.

ColemanTom · 2023-11-21T23:04:06Z

@oliver-sanders how about CHANGES.md? I can't see an 8.3.0 entry in there, or does it get automatically generated now (if so, should that tick box be removed from the list of things to tick off?)

ColemanTom · 2023-11-22T03:46:22Z

I don't really know what I'm doing with rst, but I've tried to make updates to cylc-doc

oliver-sanders · 2023-11-22T10:30:05Z

how about CHANGES.md? I can't see an 8.3.0 entry in there, or does it get automatically generated now

Use towncrier to create a changelog entry, the changelog itself gets built at release time.

The contributor page has been updated with an example command:

https://github.com/cylc/cylc-flow/blob/master/CONTRIBUTING.md#contribute-code

cylc/flow/subprocpool.py

oliver-sanders · 2023-11-23T09:45:38Z

Looks good to me.

The tests are looking a bit unhappy, I think something's gone wrong with the trigger loading, here's some traceback extracted from the functional test failure:

    Traceback (most recent call last):
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/subprocpool.py", line 86, in get_func
        mod_by_name = __import__(mod_name, fromlist=[mod_name])
    ModuleNotFoundError: No module named 'wall_clock'
    During handling of the above exception, another exception occurred:
    Traceback (most recent call last):
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/xtrigger_mgr.py", line 263, in validate_xtrigger
        func = get_func(fname, fdir)
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/subprocpool.py", line 94, in get_func
        _XTRIG_FUNCS[func_name] = entry_point.resolve()
    AttributeError: 'EntryPoint' object has no attribute 'resolve'
    During handling of the above exception, another exception occurred:
    Traceback (most recent call last):
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/scheduler.py", line 641, in run_scheduler
        await self.main_loop()
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/scheduler.py", line 1755, in main_loop
        self.process_queued_task_messages()
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/scheduler.py", line 894, in process_queued_task_messages
        self.task_events_mgr.FLAG_RECEIVED, submit_num
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/task_events_mgr.py", line 699, in process_message
        itask, event_time, self.JOB_FAILED):
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/task_events_mgr.py", line 1132, in _process_message_failed
        self._retry_task(itask, timer.timeout)
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/task_events_mgr.py", line 1098, in _retry_task
        os.getenv("CYLC_WORKFLOW_RUN_DIR")
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/xtrigger_mgr.py", line 324, in add_trig
        self.validate_xtrigger(label, fctx, fdir)
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/xtrigger_mgr.py", line 274, in validate_xtrigger
        f"'{fname}' not found in xtrigger module '{fname}'",
    cylc.flow.exceptions.XtriggerConfigError: [_cylc_retry_1/foo] 'wall_clock' not found in xtrigger module 'wall_clock'

We could also do with a functional test to ensure that the old PYTHONPATH based mechanism continues to work as it would be quite easy for us to break it accidentally now that the built-in xtriggers are properly registered via entry-points.

ColemanTom · 2023-11-27T04:39:41Z

The tests are looking a bit unhappy, I think something's gone wrong with the trigger loading, here's some traceback extracted from the functional test failure:

    Traceback (most recent call last):
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/subprocpool.py", line 86, in get_func
        mod_by_name = __import__(mod_name, fromlist=[mod_name])
    ModuleNotFoundError: No module named 'wall_clock'
    During handling of the above exception, another exception occurred:
    Traceback (most recent call last):
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/xtrigger_mgr.py", line 263, in validate_xtrigger
        func = get_func(fname, fdir)
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/subprocpool.py", line 94, in get_func
        _XTRIG_FUNCS[func_name] = entry_point.resolve()
    AttributeError: 'EntryPoint' object has no attribute 'resolve'
    During handling of the above exception, another exception occurred:
    Traceback (most recent call last):
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/scheduler.py", line 641, in run_scheduler
        await self.main_loop()
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/scheduler.py", line 1755, in main_loop
        self.process_queued_task_messages()
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/scheduler.py", line 894, in process_queued_task_messages
        self.task_events_mgr.FLAG_RECEIVED, submit_num
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/task_events_mgr.py", line 699, in process_message
        itask, event_time, self.JOB_FAILED):
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/task_events_mgr.py", line 1132, in _process_message_failed
        self._retry_task(itask, timer.timeout)
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/task_events_mgr.py", line 1098, in _retry_task
        os.getenv("CYLC_WORKFLOW_RUN_DIR")
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/xtrigger_mgr.py", line 324, in add_trig
        self.validate_xtrigger(label, fctx, fdir)
      File "/home/runner/work/cylc-flow/cylc-flow/cylc/flow/xtrigger_mgr.py", line 274, in validate_xtrigger
        f"'{fname}' not found in xtrigger module '{fname}'",
    cylc.flow.exceptions.XtriggerConfigError: [_cylc_retry_1/foo] 'wall_clock' not found in xtrigger module 'wall_clock'

Do entry points get resolved for the tests? If so, how? Is there anything I need to add to have entry points added which are defined in the setup.cfg file?

We could also do with a functional test to ensure that the old PYTHONPATH based mechanism continues to work as it would be quite easy for us to break it accidentally now that the built-in xtriggers are properly registered via entry-points.

That should be doable easily enough, but similar question to above, do I need to do anythnig special to add in the entry points to ensure they are resolved correctly?

oliver-sanders · 2023-11-27T10:57:39Z

Do entry points get resolved for the tests?

Yes, we pip install Cylc before running the tests so any commands, plugins and now xtriggers defined in the cylc-flow package will be available.

To debug, you can run the failing tests locally, e.g:

$ pytest -vv tests/unit/test_xtrigger_mgr.py::test_add_xtrigger
$ etc/bin/run-functional-tests -v  tests/f/retries/01-submission-retry.t

ColemanTom · 2023-11-30T04:51:01Z

To debug, you can run the failing tests locally, e.g:

~~I hate to say this as its very cliche, but... the tests don't fail locally for me.~~

~~I've created a conda environment from the conda-environment.yml file~~
~~I activate the conda environment~~
~~I do pip install --no-deps . from the Cylc repo~~
~~I then try tests/unit/test_xtrigger_mgr.py::test_add_xtrigger and it passes~~
~~I try one of the failing functional tests, tests/f/runahead/06-release-update.t, all tests pass~~

EDIT:

🤦‍♂️ - I didn't have my branch checked out, I had a different branch open, tests correctly fail now, will have a look

This creates a clean way to add xtriggers from python packages. xtriggers no longer need to be directly in the PYTHONPATH, but instead can be added via a cylc.xtriggers entry point. Local cylc-flow xtriggers are defined via a new entry_point.

ColemanTom · 2023-11-30T05:42:10Z

I've pushed some fixes. Not sure how it was working before this, unless I had some changes I didn't commit and lost when I did a different piece of work.

Running a few of the failed tests all had them passing now.

I'll look at an extra functional test next week (unless I can get one done within a few minutes, but I doubt it).

oliver-sanders · 2023-11-30T09:33:21Z

Please ignore tests/integration/tui test failures for now, we've been having a bad run of CI failure issues but should get things back on track soon.

ColemanTom · 2023-12-06T03:15:23Z

@oliver-sanders having a look at this, adding xtriggres via the PYTHONPATH does not work as the PYTHONPATH is removed. I'm guessing you're meaning CYLC_PYTHONPATH instead?

Also, docs on custom xtriggers probably need updating

they can be located either:

in /lib/python/;

or anywhere in your Python library path.

Ensures that xtrggers in your CYLC_PYTHONPATH are respected above entry_point xtriggers.

ColemanTom · 2023-12-06T03:46:01Z

Functional test for CYLC_PYTHONPATH added in be3e277 @oliver-sanders

MetRonnie

Thanks @ColemanTom

I've got a few suggestions

cylc/flow/subprocpool.py

tests/functional/xtriggers/04-respect-cylc-pythonpath.t

tests/functional/xtriggers/04-respect-cylc-pythonpath/flow.cylc

tests/functional/xtriggers/04-respect-cylc-pythonpath.t

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

ColemanTom · 2023-12-13T21:08:43Z

Thanks @MetRonnie - I added all your suggestions.

MetRonnie

👍

oliver-sanders · 2024-01-02T14:21:22Z

(re-opened to kick the tests)

oliver-sanders

LGTM, cheers!

* upstream/master: (350 commits) Apply suggestions from code review [skip ci] Added missing union type import (cylc#5906) set CYLC_DEBUG env var when set-verbosity DEBUG (cylc#5854) coverage: exclude report-timings and set 90% as the lower limit lint: Warn users that `CYLC_VERSION={{CYLC_VERSION}}` is deprecated (cylc#5890) Add a new entry_point for xtriggers (cylc#5831) Update copyright year Tests: fix cached datetime arithmetic contamination between tests using different calendar modes Replace cyclers functional reftests with integration reftests Simplify integration reftest Replace pre-initial functional reftests with integration reftests Integration tests: add a simpler reftest fixture auto update syntax files id: catch traceback in ID parsing actions: downgrade macos runners to macos 11 (cylc#5892) Feat.lint obsolete vars (cylc#5879) dump: restrict window to n=0 (cylc#5600) Add chained offset logic for FCP (cylc#5885) Test.example replace a reftest (cylc#5860) GH Actions artifact name fixes ...

* upstream/master: (81 commits) Apply suggestions from code review [skip ci] Added missing union type import (cylc#5906) coverage: exclude report-timings and set 90% as the lower limit lint: Warn users that `CYLC_VERSION={{CYLC_VERSION}}` is deprecated (cylc#5890) Add a new entry_point for xtriggers (cylc#5831) Tests: fix cached datetime arithmetic contamination between tests using different calendar modes Replace cyclers functional reftests with integration reftests Simplify integration reftest Replace pre-initial functional reftests with integration reftests Integration tests: add a simpler reftest fixture auto update syntax files Feat.lint obsolete vars (cylc#5879) dump: restrict window to n=0 (cylc#5600) Test.example replace a reftest (cylc#5860) GH Actions artifact name fixes Bump actions/upload-artifact from 3 to 4 Bump actions/download-artifact from 3 to 4 tests/i: update tui screenshot Update .github/workflows/test_fast.yml actions: add non-utc job to fast tests ...

ColemanTom force-pushed the entry_points_xtriggers branch from aa170b1 to fd90f6d Compare November 21, 2023 01:51

MetRonnie added the question Flag this as a question for the next Cylc project meeting. label Nov 21, 2023

MetRonnie added this to the cylc-8.x milestone Nov 21, 2023

oliver-sanders reviewed Nov 21, 2023

View reviewed changes

cylc/flow/scripts/install.py Outdated Show resolved Hide resolved

cylc/flow/subprocpool.py Outdated Show resolved Hide resolved

ColemanTom force-pushed the entry_points_xtriggers branch from fd90f6d to 065464c Compare November 21, 2023 22:54

ColemanTom mentioned this pull request Nov 22, 2023

Add documentation for xtriggers entry_point cylc/cylc-doc#671

Merged

1 task

oliver-sanders reviewed Nov 22, 2023

View reviewed changes

cylc/flow/subprocpool.py Outdated Show resolved Hide resolved

ColemanTom force-pushed the entry_points_xtriggers branch from 065464c to 3c36a6a Compare November 22, 2023 22:55

oliver-sanders assigned ColemanTom Nov 27, 2023

ColemanTom added 2 commits November 30, 2023 16:37

Add a new entry_point for xtriggers

fcc642b

This creates a clean way to add xtriggers from python packages. xtriggers no longer need to be directly in the PYTHONPATH, but instead can be added via a cylc.xtriggers entry point. Local cylc-flow xtriggers are defined via a new entry_point.

Add changes file for PR 5831

cd55e6d

ColemanTom force-pushed the entry_points_xtriggers branch from c27f21d to cd55e6d Compare November 30, 2023 05:37

Add xtrigger CYLC_PYTHONPATH functional test

be3e277

Ensures that xtrggers in your CYLC_PYTHONPATH are respected above entry_point xtriggers.

oliver-sanders removed the question Flag this as a question for the next Cylc project meeting. label Dec 6, 2023

oliver-sanders requested a review from MetRonnie December 11, 2023 12:56

oliver-sanders modified the milestones: cylc-8.x, cylc-8.3.0 Dec 11, 2023

MetRonnie reviewed Dec 13, 2023

View reviewed changes

ColemanTom and others added 5 commits December 14, 2023 08:06

Update tests/functional/xtriggers/04-respect-cylc-pythonpath.t

78f695f

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Simplify 04-respect-cylc-pythonpath flow.cylc test file

17a5266

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Update description of 04-respect-cylc-pythonpath.t

6581153

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Simplify final test in xtriggers/04-respect-cylc-pythonpath.t

badcdb1

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Improve docstring for xtrigger location order specification

c08996f

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

ColemanTom mentioned this pull request Dec 14, 2023

cylc config shows unhelpful information for xtriggers #5877

Closed

MetRonnie approved these changes Dec 14, 2023

View reviewed changes

oliver-sanders closed this Jan 2, 2024

oliver-sanders reopened this Jan 2, 2024

oliver-sanders approved these changes Jan 2, 2024

View reviewed changes

oliver-sanders merged commit b8a55d8 into cylc:master Jan 2, 2024
35 of 38 checks passed

This was referenced Jan 26, 2024

Xtrigger function arg validation. #5452

Closed

added suite state xtrigger for back-compat #5864

Merged

oliver-sanders mentioned this pull request Feb 1, 2024

plugins: xtriggers #3456

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new entry_point for xtriggers #5831

Add a new entry_point for xtriggers #5831

ColemanTom commented Nov 21, 2023 •

edited

Loading

ColemanTom commented Nov 21, 2023

oliver-sanders left a comment

oliver-sanders commented Nov 21, 2023

oliver-sanders commented Nov 21, 2023

ColemanTom commented Nov 21, 2023

ColemanTom commented Nov 21, 2023

ColemanTom commented Nov 21, 2023

ColemanTom commented Nov 22, 2023

oliver-sanders commented Nov 22, 2023

oliver-sanders commented Nov 23, 2023

ColemanTom commented Nov 27, 2023

oliver-sanders commented Nov 27, 2023 •

edited

Loading

ColemanTom commented Nov 30, 2023 •

edited

Loading

ColemanTom commented Nov 30, 2023

oliver-sanders commented Nov 30, 2023

ColemanTom commented Dec 6, 2023 •

edited

Loading

ColemanTom commented Dec 6, 2023

MetRonnie left a comment

ColemanTom commented Dec 13, 2023

MetRonnie left a comment

oliver-sanders commented Jan 2, 2024

oliver-sanders left a comment

Add a new entry_point for xtriggers #5831

Add a new entry_point for xtriggers #5831

Conversation

ColemanTom commented Nov 21, 2023 • edited Loading

ColemanTom commented Nov 21, 2023

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders commented Nov 21, 2023

oliver-sanders commented Nov 21, 2023

ColemanTom commented Nov 21, 2023

ColemanTom commented Nov 21, 2023

ColemanTom commented Nov 21, 2023

ColemanTom commented Nov 22, 2023

oliver-sanders commented Nov 22, 2023

oliver-sanders commented Nov 23, 2023

ColemanTom commented Nov 27, 2023

oliver-sanders commented Nov 27, 2023 • edited Loading

ColemanTom commented Nov 30, 2023 • edited Loading

ColemanTom commented Nov 30, 2023

oliver-sanders commented Nov 30, 2023

ColemanTom commented Dec 6, 2023 • edited Loading

ColemanTom commented Dec 6, 2023

MetRonnie left a comment

Choose a reason for hiding this comment

ColemanTom commented Dec 13, 2023

MetRonnie left a comment

Choose a reason for hiding this comment

oliver-sanders commented Jan 2, 2024

oliver-sanders left a comment

Choose a reason for hiding this comment

ColemanTom commented Nov 21, 2023 •

edited

Loading

oliver-sanders commented Nov 27, 2023 •

edited

Loading

ColemanTom commented Nov 30, 2023 •

edited

Loading

ColemanTom commented Dec 6, 2023 •

edited

Loading