-
Notifications
You must be signed in to change notification settings - Fork 113
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
Conversation
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
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 |
There was a problem hiding this comment.
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 <[email protected]>
There was a problem hiding this 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 <[email protected]>
There was a problem hiding this 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 <[email protected]>
…kedro-viz into feature/include-hooks Signed-off-by: ravi-kumar-pilla <[email protected]>
There was a problem hiding this 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?
Signed-off-by: ravi-kumar-pilla <[email protected]>
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 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 |
Apologies for the delayed response. We are using some kedro starters we've
developed to help medium-maturity users get going faster on projects. We
are using DataCatalog hooks in several projects and also kedro boot in
another (another framework that uses plugins/hooks)
https://github.com/takikadiri/kedro-boot ; in both cases the users now see
scary warning messages when they run `kedro viz` with kedro viz 9 because
the hooks are needed to resolve parameters/datasets at runtime. We have
updated our readmes to use the new --include-hooks flag, but definitely
folks will now need to learn the new way of starting viz - not really a big
deal for folks who are comfortable/developing every day, but definitely
more words to type and different from how things were. Hopefully that helps
provide some context - not sure if there is a great solution, but hopefully
it's helpful to share an example of how the change in default behavior gets
perceived by users.
One other thing to mention as a wrinkle - we had our starters pinned to
kedro viz 8, but mainline kedro dropping toposort now causes problems with
newer versions of kedro mixed with kedro viz 8. We're tentatively thinking
we keep kedro viz unpinned going forward, and have updated our instructions
for new users to use the --include-hooks flag
…On Wed, May 1, 2024 at 3:32 PM Ravi Kumar Pilla ***@***.***> wrote:
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 <https://github.com/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
—
Reply to this email directly, view it on GitHub
<#1818 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD2Y5QZW67TKVNGSXRNBGLZAFGMBAVCNFSM6AAAAABFJJMKSKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBZGA4DMNBSGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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 Thank you |
Description
Resolves #1808
Development notes
--include-hooks
option to all the viz cli commands (run, build, deploy)--include-hooks
option to%run_viz
Jupyter line magic--ignore-plugins
option from viz cli commandsQA notes
--include-hooks
should appear as an option with -h (eg.,kedro viz run -h
)Checklist
RELEASE.md
file