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.

Sign up for GitHub

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

Jump to bottom

Execution hooks (core features) #508

Merged
merged 157 commits into from
Dec 29, 2021
Merged

Execution hooks (core features) #508

merged 157 commits into from
Dec 29, 2021

Conversation

bdice
Copy link
Member

@bdice bdice commented Apr 30, 2021 •
edited by klywang
Loading

Description

Extension of #189 that includes only core features. This will make the PR easier to review and finalize. See #189 for details.

To-do:

  • Update test class structure, see comment.
  • Add tests that improve coverage.
  • Discuss API for hooks. What should the signature be? Currently hook(operation_name, error, *jobs) (error only for on_fail).
  • Update docs.

Motivation and Context

Implement core features of execution hooks.

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality.

Checklist:

If necessary:

  • I have updated the API documentation as part of the package doc-strings.
  • I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
  • I have updated the changelog.

csadorf added 30 commits December 5, 2019 19:49
@csadorf
Prototype implementation for execution hooks.
d1c9158 
This is a prototype implementation for the API proposed in issue #28.
@csadorf
Fix issue that occurs when the add_operation 'hooks' argument is None.
75fa4e5
@csadorf
Properly handle exceptions for on_start hooks.
fb61dd4
@csadorf
Reduce verbosity of hook module on errors.
a3942ce
@csadorf
Implement unit tests for run execution hooks.
4a8192f
@csadorf
Revised hooks implementation and provide basic hook modules.
6caf1c0
@csadorf
Fix docstrings in snapshots module (typo/style).
e066d45
@csadorf
Add option to exclude files/directories based on regular expression.
3ca7655 
In the snapshot hook module.
@csadorf
Rename the 'track-operations' executable to 'flow-track-operations'.
6b55bca
@csadorf
Add fork execution directive.
c1b4f3d
@csadorf
Remove fork execution directive.
652ed87
@csadorf
Fix rebase error.
4232c06
@csadorf
Install hooks through config as callable.
201b7f1 
To avoid the need to implement a class and also avoid forcing of a
specific function name.
@csadorf
Use hidden files for the operations log and tracking log.
130e85d 
This makes it easier to integrate those hook systems with git
tracking.
@csadorf
Rename '_metadata_schema_version' to '_schema_version'.
b03d211
@csadorf
Remove left-over print statement.
460e1e7
@csadorf
Use git-notes for automated git-tracking.
03fd426 
Instead of storing metadata in the commit message.
@csadorf
Fix style error.
ca207d9
@csadorf
Revert changes to base submission script.
5602701
@csadorf
Update changelog.
4f21e6a
@csadorf
Remove obselete internal function.
6c93fdc
@csadorf
Update metadata schema.
f05d45e
@csadorf
Remove print_function from __future__ import.
6d8afef 
No longer needed since the drop of support for Python 2.7.
@csadorf
fixup! Update metadata schema.
556446e
@csadorf
Rename '.snapshots' directory to '.signac_snapshots'.
28f621a
@csadorf
Refactor git repot initialization and git-ignore handling.
860abd6 
Also just ignore the hidden signac snapshots directory instead of
all hidden files and directories.
@csadorf
Use improved storage layout for snapshot system.
074cb5b
@csadorf
changes I failed to commit at the time
05e0a81
@csadorf
Merge branch 'master' into execution-hooks
20dcb32
@csadorf
Fix issue with test_context unit test.
40169c8
@csadorf
Copy link
Contributor

csadorf commented Nov 17, 2021 

1. Should `flow.hooks.Hooks` be public? Should it be documented in our API? It is never user-facing as far as I can tell.

No, the properties on the project that users are expected to use should be public and part of the API IMO.

2. We need to fix the docs for `FlowProject.operation_hooks`. I noticed it wasn't documented at all so I added it to `api.rst` but it's currently broken (shows up as "alias of `flow.project._FlowProjectClass._setup_hook_object.<locals>._HooksRegister`"). I think this used to be an issue with pre/post so a similar fix may be applicable (I can't remember what I did).

What is your suggestion? Should we try to fix this as part of this PR or not?

@bdice
Copy link
Member Author

bdice commented Nov 17, 2021 •
edited
Loading

  1. Should flow.hooks.Hooks be public? Should it be documented in our API? It is never user-facing as far as I can tell.

No, the properties on the project that users are expected to use should be public and part of the API IMO.

I agree. I recommend renaming Hooks to _Hooks and making FlowProject.project_hooks / FlowProject.operation_hook the public documented API.

  1. We need to fix the docs for FlowProject.operation_hooks. I noticed it wasn't documented at all so I added it to api.rst but it's currently broken (shows up as "alias of flow.project._FlowProjectClass._setup_hook_object.<locals>._HooksRegister"). I think this used to be an issue with pre/post so a similar fix may be applicable (I can't remember what I did).

What is your suggestion? Should we try to fix this as part of this PR or not?

I'm fine with fixing the docs in this PR or in a subsequent PR -- just wanted to note the issue for right now. I can revisit this again later in the week but would welcome anyone who has time to work on it sooner.

@b-butler
Copy link
Member

@bdice I couldn't get the documentation quite right but I made it passable I think.

b-butler
b-butler reviewed Nov 22, 2021
View reviewed changes
flow/project.py Outdated Show resolved Hide resolved
b-butler
b-butler reviewed Nov 22, 2021
View reviewed changes
flow/project.py Outdated Show resolved Hide resolved
b-butler
b-butler approved these changes Nov 22, 2021
View reviewed changes
Copy link
Member

@b-butler b-butler left a comment

Choose a reason for hiding this comment

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

👍

klywang
klywang approved these changes Nov 22, 2021
View reviewed changes
Copy link
Contributor

@klywang klywang left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@bdice
Copy link
Member Author

bdice commented Dec 5, 2021

I have resolved conflicts with other recently merged PRs (template reference data was updated). @csadorf If you would like to offer a final review, this is ready. Otherwise I think we can merge this PR.

@bdice bdice merged commit 54c47dd into master Dec 29, 2021
@bdice bdice deleted the feature/hooks-core branch December 29, 2021 14:10
@bdice bdice mentioned this pull request Dec 29, 2021
Closed
@cbkerr cbkerr added this to the v0.18.0 milestone Feb 3, 2022
@cbkerr cbkerr mentioned this pull request Feb 22, 2022
Merged
6 tasks
@b-butler b-butler mentioned this pull request Apr 18, 2022
Open
@klywang klywang mentioned this pull request Aug 2, 2022
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klywang klywang klywang approved these changes

@b-butler b-butler b-butler approved these changes

@csadorf csadorf Awaiting requested review from csadorf

Assignees

@cbkerr cbkerr

@klywang klywang

Labels
enhancement New feature or request hooks
Projects
None yet
Milestone
v0.18.0
Development

Successfully merging this pull request may close these issues.
6 participants
@bdice @klywang @csadorf @b-butler @cbkerr @vyasr