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

Feature: Add LogOperations to execution hooks #661

Open
wants to merge 48 commits into
base: main
Choose a base branch
from

Conversation

klywang
Copy link
Contributor

@klywang klywang commented Aug 2, 2022

Add builtin logging for execution hooks.

Description

Add LogOperations and tests for LogOperations to execution hooks.

Motivation and Context

Provides built in logging hooks for users. Partially addresses #637 and #508.

Checklist:

@codecov
Copy link

codecov bot commented Aug 2, 2022

Codecov Report

Merging #661 (672726b) into main (02fc644) will increase coverage by 0.26%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #661      +/-   ##
==========================================
+ Coverage   69.28%   69.55%   +0.26%     
==========================================
  Files          44       45       +1     
  Lines        4295     4332      +37     
  Branches     1047     1051       +4     
==========================================
+ Hits         2976     3013      +37     
  Misses       1108     1108              
  Partials      211      211              
Files Coverage Δ
flow/hooks/__init__.py 100.00% <100.00%> (ø)
flow/hooks/hooks.py 88.23% <ø> (ø)
flow/hooks/log_operations.py 100.00% <100.00%> (ø)

klywang and others added 20 commits August 2, 2022 12:17
Names now match the names of the hook triggers. We also rename the
install_hooks to specify that it installs hooks project wide.
The call method is confusing as it only applies to projects but hooks
can be project or operation wide.
Allow for the correct passing of the project to install the hooks for.
Assumes that we are using different log files in LogOperation tests.
@b-butler b-butler marked this pull request as ready for review October 13, 2022 17:57
@b-butler b-butler requested review from a team as code owners October 13, 2022 17:57
@b-butler b-butler requested review from b-butler and SchoeniPhlippsn and removed request for a team and b-butler October 13, 2022 17:57
@b-butler b-butler added this to the v1.0 milestone Feb 22, 2023
@vyasr
Copy link
Contributor

vyasr commented Jun 21, 2023

Same as the question on #739, @klywang what is this PR waiting on? Reviews? Do we need to ping people to have a look, or is there more work to be done first?

@klywang
Copy link
Contributor Author

klywang commented Jun 21, 2023

Same as the question on #739, @klywang what is this PR waiting on? Reviews? Do we need to ping people to have a look, or is there more work to be done first?

@vyasr It's waiting on a review.

@kidrahahjo If you have time, could you take a look at this PR again?

flow/hooks/hooks.py Outdated Show resolved Hide resolved
flow/hooks/log_operations.py Outdated Show resolved Hide resolved
flow/hooks/log_operations.py Outdated Show resolved Hide resolved
flow/hooks/log_operations.py Show resolved Hide resolved
flow/hooks/log_operations.py Outdated Show resolved Hide resolved
flow/hooks/log_operations.py Outdated Show resolved Hide resolved
flow/hooks/log_operations.py Outdated Show resolved Hide resolved
flow/hooks/log_operations.py Outdated Show resolved Hide resolved
flow/hooks/log_operations.py Outdated Show resolved Hide resolved
flow/hooks/log_operations.py Outdated Show resolved Hide resolved
Copy link
Contributor

@csadorf csadorf 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! Just a few minor suggestions.

flow/hooks/log_operations.py Outdated Show resolved Hide resolved
Comment on lines 63 to 64
# Do something

Copy link
Contributor

Choose a reason for hiding this comment

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

Since many users typically just copy any example code verbatim, this is meant to emphasize that without a Project definition, nothing would actually happen. So an alternative would be to add at least one "hello world" operation to the example or make this comment a bit clearer, e.g., # Project operation definitions ...".

tests/test_project.py Outdated Show resolved Hide resolved
flow/hooks/log_operations.py Outdated Show resolved Hide resolved
klywang and others added 11 commits July 7, 2023 12:14
Bumps [pre-commit](https://github.com/pre-commit/pre-commit) from 3.3.2 to 3.3.3.
- [Release notes](https://github.com/pre-commit/pre-commit/releases)
- [Changelog](https://github.com/pre-commit/pre-commit/blob/main/CHANGELOG.md)
- [Commits](pre-commit/pre-commit@v3.3.2...v3.3.3)

---
updated-dependencies:
- dependency-name: pre-commit
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [ruamel-yaml](https://sourceforge.net/p/ruamel-yaml/code/ci/default/tree) from 0.17.31 to 0.17.32.

---
updated-dependencies:
- dependency-name: ruamel-yaml
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [pytest](https://github.com/pytest-dev/pytest) from 7.3.1 to 7.4.0.
- [Release notes](https://github.com/pytest-dev/pytest/releases)
- [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)
- [Commits](pytest-dev/pytest@7.3.1...7.4.0)

---
updated-dependencies:
- dependency-name: pytest
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
updates:
- [github.com/asottile/pyupgrade: v3.3.1 → v3.8.0](asottile/pyupgrade@v3.3.1...v3.8.0)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@b-butler b-butler force-pushed the feature/hooks-LogOperation branch from f4190f9 to 6287212 Compare October 13, 2023 14:52
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.

I think this is very close. We should push this through before the next release.

flow/hooks/log_operations.py Show resolved Hide resolved
flow/hooks/log_operations.py Show resolved Hide resolved
@b-butler b-butler force-pushed the feature/hooks-LogOperation branch from 2132486 to 672726b Compare October 13, 2023 18:05
@b-butler b-butler requested a review from csadorf October 20, 2023 18:54
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.

7 participants