Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Visualize pipeline objects in notebook #2241
base: main
Are you sure you want to change the base?
Visualize pipeline objects in notebook #2241
Changes from 42 commits
dc31929
d3448dc
7755e11
7c264dd
bf08766
4483342
fd532ee
563182f
e8f7249
8b66fec
72bcc28
32632d0
d3f9c21
c148a55
f7e10a1
5031722
06fc82d
8298408
6e5511b
b49109b
fd379f7
d962a9b
7ad4be9
47e2b4b
199a34c
45dd808
4f69d7e
27cfd9d
0485f45
508beaa
fac1b3c
ffe1657
08f74e8
4cf8635
be2d9d8
450a695
6f4fcc3
cdc2d7a
847bc95
6ff45ed
1115c34
05ce8de
fb73d92
28a228f
884752c
67cfa5f
bb29abf
f956451
14c6c7a
8d79192
b494af5
8678052
84f1e07
e7b5239
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 12 in RELEASE.md
[vale] RELEASE.md#L12
Raw output
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 should probably not be there but in intergrations/notebook/init.py then users can do
from kedro_viz.intergrations.notebook import NotebookVisualizer
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 like the idea. I don't have a strong opinion on this. I wanted users to get the
NotebookVisualizer
class easily, but I can move it too.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 is an experimental feature, and we know that only a very small percentage of users run Kedro-Viz in notebooks—especially since run_viz was broken for months! If we import it here, it will load every time a user runs Kedro-Viz, even when notebooks aren’t involved, which isn’t ideal.
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.
Can we make session_store and stat_dict optional properties in populate_data? We're already removing session_store in the ET removal PR, and stat_dict doesn’t seem essential.
Not sure if we should do the same for catalog—do we actually need it in the notebook visualizer?
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.
Hi @rashidakanchwala , I think we need DataCatalog to be not optional as we do add_catalog in the following steps and we expect Catalog to be present. For this PR, I am not making the changes to populate_data as it is introducing other changes in DataAccessManager, conftest and other test cases. It would be better we handle this in other ticket which involves moving these methods from server.py as we discussed. wdyt ?