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

Add runner class name to pipeline hooks + introduce after_command_run CLI hook spec #1309

Merged
merged 34 commits into from
Apr 21, 2022
Merged

Add runner class name to pipeline hooks + introduce after_command_run CLI hook spec #1309

merged 34 commits into from
Apr 21, 2022

Conversation

datajoely
Copy link
Contributor

@datajoely datajoely commented Mar 3, 2022 •
edited
Loading

Description

This PR adds some functionality to 3 existing hooks and introduces one new CLI hook:

Modified hooks:

  • before_pipeline_run
  • after_pipeline_run
  • on_pipeline_error

The runner class name is now included within the run_params dictionary.

New hooks:

  • after_command_run (CLI hook)

This has been requested several times over the last year or so the two usecases that come to mind are:

  1. Ability to tell telemetry which commands actually worked and which failed
  2. The developer of kedro-dvc specifically asked to expose this so that they can install their own configuration once kedro new is executed.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

datajoely and others added 14 commits March 3, 2022 08:53
after/before pipeline hook updates
d1b2489 
Signed-off-by: datajoely <[email protected]>
Fix mypy
718c771 
Signed-off-by: datajoely <[email protected]>
black
145e8b5 
Signed-off-by: datajoely <[email protected]>
Merge remote-tracking branch 'origin/develop' into feature/add-more-h…
500601c 
…ook-specs

Signed-off-by: datajoely <[email protected]>
linting
30aa134 
Signed-off-by: datajoely <[email protected]>
typing issues
fcc296d 
Signed-off-by: datajoely <[email protected]>
Fix mock
9f52f2d 
Signed-off-by: datajoely <[email protected]>
@datajoely
Merge branch 'develop' into feature/add-more-hook-specs
f51c9ff
patch runner
f8e9fb2 
Signed-off-by: datajoely <[email protected]>
fix tests
edeed9f 
Signed-off-by: datajoely <[email protected]>
Introdce after_cli_run plugin hook
68d0061 
Signed-off-by: datajoely <[email protected]>
@datajoely
Merge branch 'develop' into feature/add-more-hook-specs
b07a957
Update docs
85ea025 
Signed-off-by: datajoely <[email protected]>
Add release notes
fe40424 
Signed-off-by: datajoely <[email protected]>
@datajoely datajoely changed the title Extend current hook specs to provide more functionality Improve pipeline hook specs and introduce new CLI hook spec Mar 3, 2022
@datajoely
Copy link
Contributor Author

Opted to provide the name of the runner class to avoid circular import, but the user value is identical

@datajoely datajoely marked this pull request as ready for review March 3, 2022 18:54
merelcht
merelcht reviewed Mar 4, 2022
View reviewed changes
RELEASE.md Outdated Show resolved Hide resolved
@datajoely datajoely changed the title Improve pipeline hook specs and introduce new CLI hook spec [FOR DISCUSSION] Improve pipeline hook specs and introduce new CLI hook spec Mar 10, 2022
datajoely added 8 commits April 11, 2022 11:38
Merge remote-tracking branch 'origin/main' into feature/add-more-hook…
7d4fb03 
…-specs

Signed-off-by: datajoely <[email protected]>
remove lint disabler
ecc7dc9 
Signed-off-by: datajoely <[email protected]>
Revert changes
2c0ef38 
Signed-off-by: datajoely <[email protected]>
trailing whitespace
9b3645a 
Signed-off-by: datajoely <[email protected]>
fix test
4bbd5c5 
Signed-off-by: datajoely <[email protected]>
Fix lint issue
c8e97f6 
Signed-off-by: datajoely <[email protected]>
Fix tests
21e251f 
Signed-off-by: datajoely <[email protected]>
black
47940a6 
Signed-off-by: datajoely <[email protected]>
@datajoely datajoely changed the base branch from develop to main April 11, 2022 15:01
@datajoely datajoely changed the title [FOR DISCUSSION] Improve pipeline hook specs and introduce new CLI hook spec Add runner class name to pipeline hooks + introduce after_command_run CLI hook spec Apr 11, 2022
merelcht
merelcht approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM! Much cleaner than before 👍
I've got some minor comments and questions, but nothing to block merging.

kedro/framework/session/session.py Show resolved Hide resolved
tests/framework/cli/test_cli_hooks.py Show resolved Hide resolved
tests/framework/cli/test_cli_hooks.py Show resolved Hide resolved
kedro/framework/cli/cli.py Outdated Show resolved Hide resolved
kedro/framework/cli/cli.py Outdated Show resolved Hide resolved
idanov
idanov approved these changes Apr 20, 2022
View reviewed changes
Copy link
Member

@idanov idanov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Happy the API surface doesn't increase by much. Probably should've been done in two separete PRs, but let's not slow down the merge further.

@datajoely datajoely self-assigned this Apr 20, 2022
@datajoely datajoely mentioned this pull request Apr 21, 2022
Closed
@merelcht merelcht merged commit a7519b7 into main Apr 21, 2022
@merelcht merelcht deleted the feature/add-more-hook-specs branch April 21, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merelcht merelcht merelcht approved these changes

@idanov idanov idanov approved these changes

@yetudada yetudada Awaiting requested review from yetudada yetudada is a code owner

@lorenabalan lorenabalan Awaiting requested review from lorenabalan

@antonymilne antonymilne Awaiting requested review from antonymilne

Assignees

@datajoely datajoely

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@datajoely @idanov @merelcht