-
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
Set Kedro Viz start method to spawn process while launching it from Jupyter Notebook #1696
Conversation
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
Signed-off-by: ravi-kumar-pilla <[email protected]>
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. thanks
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 fine to me, but my question is does it matter to viz to have the Spark context initialised? Is it being used to read data?
Ravi would have more clarity on this but from what it looks like is that if Spark context is not intiatilised; the spark dataset is loaded as a MemoryDataSet on viz. |
Hi Nok, It should not matter to viz but in Viz 6.7.0 we introduced a dataset exists check which forced the discovery of all the datasets. Since on unix forked process is default, this check caused an issue as viz process is a copy. Starting Viz as a new process would not have this dependency. The issue is explained here in detail. Also, for your information the dataset exists check was removed in v7.0.0 as the check introduced other time out issues. So the Spark re-initialization issue should be solved in v7.0.0. However, this change will force viz to always start as a new process irrespective of the OS. Thank you |
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 👍
Description
Resolves #1656
Development notes
add_route
andadd_websocket_route
due to strict mypy check introduced here in starletteQA notes
Checklist
RELEASE.md
file