-
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
Fix for Kedro Viz Connection Error #1507
Conversation
…x/connection-error
…x/connection-error
…x/connection-error
…edro-viz into fix/connection-error
…x/connection-error
…1499) * add stats to data node * lint and format check fix * fix pytests * fix layout issue * fix transcoded data stats Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
* initial draft for python 3.11 support * update release doc * add python warnings for e2e tests * modify e2e test * modify e2e test * test by removing lower req scenario * skip e2e tests for lower bound requirement on python 3.11 * skip e2e tests for lower bound requirement on python 3.11 * remove print statements --------- Co-authored-by: Nok Lam Chan <nok_lam_chan@mckinsey.com> Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
* initial draft for python 3.11 support * update release doc * add python warnings for e2e tests * modify e2e test * modify e2e test * test by removing lower req scenario * skip e2e tests for lower bound requirement on python 3.11 * skip e2e tests for lower bound requirement on python 3.11 * remove python upperbounds initial draft * fix lint and format errors * test remove upperbound warning * test lowerbound pandas install * revert back pandas requirement * bump lower requirements for pandas * remove upper bound clean up * update release notes * fix PR comments --------- Co-authored-by: Nok Lam Chan <nok_lam_chan@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>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
* Fix: Adding favicon to Kedro Demo * Fix: Change in approach for serving favicon * Lint error fix * Lint error fix * Favicon endpoint test added * Favicon endpoint test added * Lint error fixed * Fix: Adding favicon to Kedro Demo Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com> * Fix: Change in approach for serving favicon Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com> * Lint error fix Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com> * Lint error fix Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com> * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com> * Favicon endpoint test added Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com> * Lint error fixed Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com> * Fixed favicon endpoint test * Release doc updated * Update RELEASE.md Co-authored-by: rashidakanchwala <37628668+rashidakanchwala@users.noreply.github.com> * Removed pytest.fixture as per review comment --------- Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com> Co-authored-by: rashidakanchwala <37628668+rashidakanchwala@users.noreply.github.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>
* v6.5.0 * release * update-reminder-content * update reminder 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>
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.
Leave a small comment. So to check with my understanding, the main change here is moving the start_browser
from run_server
to the cli, and the run_server only handle the backend process.
I have a quick question, why multiprocessing
is needed?
@@ -175,7 +116,7 @@ def run_viz(port: int = None, local_ns: Dict[str, Any] = None) -> None: | |||
viz_process.start() | |||
_VIZ_PROCESSES[port] = viz_process | |||
|
|||
_wait_for(func=_check_viz_up, host=host, port=port) | |||
wait_for(func=check_viz_up, host=host, port=port) |
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.
What is the reason to change from private function to public one?
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.
Earlier these functions were inside jupyter.py and used only by jupyter, now it is also used in cli launcher. so moved it to a utils file and these functions are now used in both places
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 see they are now shared among two modules, but I think it should be an internal method _wait_for
instead of wait_for
.
For example, we have similar internal utils here:
https://github.com/kedro-org/kedro/blob/edbbcc7f7a863b7c980f9efcaa444e2fa01f45fa/kedro/config/common.py#L209
Changing from _wait_for
-> wait_for
singals this is a public function. (though in reality 99% people are using kedro-viz as an appllication but not a module). So this is just to keep things clean internally for ourselves.
package/kedro_viz/launchers/cli.py
Outdated
if browser: | ||
start_browser(host, port) |
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.
👍🏼
Yes, I have moved the start_browser logic out of run_server as run_server should just focus on starting the FAST API server.
So, earlier we used to start the browser without waiting for backend server to start. Now if one process is used to start the server, there should be another process which can wait for the server to start and then start a browser (This can also be done using threading, but since we were using multiprocessing in jupyter already, I thought of using the same in cli launcher as well. ) |
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've experienced this issue, definitely an improvement for UX. Great fix, 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.
LGTM. thanks :)
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Description
Resolves #1277
Development notes
QA notes
fix/connection-error
make build
pip install -e package
kedro viz
orkedro viz --autoreload
Checklist
RELEASE.md
file