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

Introduce --include-hooks option and remove --ignore-plugins in Kedro-Viz #1818

Merged
merged 10 commits into from
Apr 4, 2024

Conversation

ravi-kumar-pilla
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla commented Mar 26, 2024

Description

Resolves #1808

Development notes

  • Added --include-hooks option to all the viz cli commands (run, build, deploy)
  • Added --include-hooks option to %run_viz Jupyter line magic
  • Removed --ignore-plugins option from viz cli commands
  • Updated pytests

QA notes

  • All tests should pass
  • --include-hooks should appear as an option with -h (eg., kedro viz run -h)

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 new entries to the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
@ravi-kumar-pilla
Copy link
Contributor Author

CircleCI Build fix - #1819

README.md Outdated

--load-file TEXT Load Kedro-Viz using JSON files from the specified
directory.
--save-file FILE Save all API responses from the backend as JSON
Copy link
Contributor

@rashidakanchwala rashidakanchwala Apr 2, 2024

Choose a reason for hiding this comment

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

since this is more user facing I was thinking maybe we stick to the old
'Save Kedro-Viz data as JSON files in the specified directory'

Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

Looks great to me !! thanks @ravi-kumar-pilla

Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Copy link
Contributor

@SajidAlamQB SajidAlamQB left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @ravi-kumar-pilla!

Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
…kedro-viz into feature/include-hooks

Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
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.

The code itself looks good, but I just wanted to clarify if --ignore-plugins is meant to be deprecated, like it says in the release notes, or removed. And if it's removed will it be included in a breaking release?

RELEASE.md Outdated Show resolved Hide resolved
@ravi-kumar-pilla ravi-kumar-pilla changed the title Introduce --include-hooks option and deprecate --ignore-plugins in Kedro-Viz Introduce --include-hooks option and remove --ignore-plugins in Kedro-Viz Apr 4, 2024
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
@ravi-kumar-pilla ravi-kumar-pilla merged commit 3c4197f into main Apr 4, 2024
14 checks passed
@ravi-kumar-pilla ravi-kumar-pilla deleted the feature/include-hooks branch April 4, 2024 14:38
@jitu5 jitu5 mentioned this pull request Apr 16, 2024
5 tasks
@brau0300
Copy link

brau0300 commented May 1, 2024

This (as expected it seems) was a breaking change for any project using hooks or plugins and was non-trivial to chase down.

It also makes it less convenient for especially new users on a project needing this flag to start kedro viz; would we be open to considering ways to pass this option (now required for certain projects) by default (e.g. via project configuration) and/or more succinctly?

@ravi-kumar-pilla
Copy link
Contributor Author

This (as expected it seems) was a breaking change for any project using hooks or plugins and was non-trivial to chase down.

It also makes it less convenient for especially new users on a project needing this flag to start kedro viz; would we be open to considering ways to pass this option (now required for certain projects) by default (e.g. via project configuration) and/or more succinctly?

Hi @brau0300 ,

Thank you for the comment. Ideally Kedro-Viz does not need any hook to start. Users should be able to do kedro viz run without any issue. Having said that, could you please let us know the use case where you need a hook to be enabled for kedro viz to start ?

At this moment, we do not have any discussions on making this available via configuration of some sort. But we would be happy to hear from you the pain points and the issue this would solve for Kedro-Viz. Thank you

@brau0300
Copy link

brau0300 commented May 9, 2024 via email

@ravi-kumar-pilla
Copy link
Contributor Author

Hi @brau0300 ,

Thank you for the response. The idea behind ignoring hooks by default was to make Kedro-Viz faster and work towards #1159 . I agree it is a breaking change, but the team decided to load Kedro-Viz with bare minimum of what it needs and users can opt in hooks if they want any additional functionality.

For the issue of dropping toposort, this could have been avoided if Kedro-Viz refrained from using transitive dependencies. We have created a ticket to make Kedro-Viz better and avoid any such instances in future.

Thank you

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.

Introducing --include-hooks parameter and deprecating --ignore-plugins in Kedro-Viz
5 participants