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

Add documentation for xtriggers entry_point #671

Merged
merged 3 commits into from
Jan 24, 2024

Conversation

ColemanTom
Copy link
Contributor

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.

I've attempted to add documentation for new functionality provided by cylc/cylc-flow#5831. I hope it works properly. I think there should be a link between the external triggers documentation and the plugin documentation, but I'm not sure how to do that. Very specific advice would be appreciated.

When I tried to build this as myself I had issues and have not had time to try and debug, so I'm not sure how it renders (or if it is even valid).

:template: docstring_only.rst

cylc.flow.xtriggers.echo
cylc.flow.xtriggers.wall_clock
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the fly in the ointment here is that the wall_clock function has a different signature in the code compared to real usage. Not sure what the reason for this was, but it was done in cylc/cylc-flow#4511. Maybe @hjoliver will be able to shed some light

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand this, outside my area of knowledge. Do I need to change anything or await advice from @hjoliver ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good if Hilary could chime in, also I seem to remember he has a draft PR that somewhat rewrites the function

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wall_clock xtrigger isn't really an xtrigger because it would be inefficient to call this function in a subprocess over and over. So instead it's actually run synchronously within the main code, the trigger time is computed from the offset and the cycle point in advance to avoid excess computation.

This sort of problem will go away with cylc/cylc-flow#3497 as this will avoid polling a function allowing it to maintain local state.

Until then, maybe just subtract the wall_clock function from the autodocumented reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, removed wall_clock. I also noticed I had PYTHONPATH instead of CYLC_PYTHONPATH, so I updated that too.

@MetRonnie MetRonnie added this to the 8.3.0 milestone Nov 29, 2023
ColemanTom and others added 2 commits January 3, 2024 09:50
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
@ColemanTom ColemanTom force-pushed the entry_points_xtriggers branch from 5c21b65 to 368818c Compare January 2, 2024 22:50
@ColemanTom
Copy link
Contributor Author

test failure is casued by openbsd website being down. Nothing I can do to fix that. It could change to the freebsd version of the ssh_config documentation, which may be a more reliable website given its customers?

@MetRonnie
Copy link
Member

MetRonnie commented Jan 15, 2024

Could you add src/plugins/xtriggers/built-in to .gitignore please (autodoc generates files into this directory)

@ColemanTom
Copy link
Contributor Author

Could you add src/plugins/xtriggers/built-in to .gitignore please (autodoc generates files into this directory)

Done

@MetRonnie
Copy link
Member

@oliver-sanders Are you happy with my single review or did you want to re-review?

@oliver-sanders oliver-sanders merged commit 7b8d510 into cylc:master Jan 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants