-
Notifications
You must be signed in to change notification settings - Fork 115
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
Skip optional plugins while running Kedro Viz #1544
Conversation
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Hi @tynandebold , @rashidakanchwala, @noklam This PR is still in draft phase (I need to add pytests for the same). Before I do that, I would like to get some feedback with the approach here. This is from Nok's suggestion here To summarize, I have added an additional option of |
Comments:
|
Yes it is true. But, I felt we should have kedro telemetry to collect the data during a kedro viz run. I will confirm this with @tynandebold
In future when vizro integration is complete, we will have it as plugin and there should be some way to not ignore few mandatory plugins.
I need to set this up locally. I will get in touch with Tynan or if someone points me to the doc for setup. Thank you |
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Hi @noklam , What you said is right regarding kedro telemetry. I can see network requests even after unregistering the hook. I think the data is collected from the kedro side and viz actually does not need the plugin. I have modified the code to initialize But I am keeping the --ignore-plugins flag default to |
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
…ature/skip-hooks Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
…ro-viz into feature/skip-hooks Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
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.
I left some small comments, but in general this approach looks good 👍
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
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. thanks. The tests are failing because of the kedro-datasets ticket..so we can merge that first.
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.
This looks good to me.
In the future, we could add something to the docs, akin to this. We don't do that with every CLI option we have though, so for now, this is good to go.
Also, please don't forget to add a line to the release notes. |
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Description
Resolves #1159
Development notes
QA notes
pip install -e package
cd demo-project
pip install kedro-mlflow
kedro viz
kedro viz --ignore-plugins
Checklist
RELEASE.md
file