-
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
Kedro CLI startup time made shorter #1196
Conversation
6189acb
to
4e58abf
Compare
I see the unit tests are failing, would you be able to have a look at it? |
Sure, I'll take a look at it in the evening. Unit tests mostly fail because of broken mocks - What about linter rules? Should I do something about them? Local import is needed for this kind of behavior but linter reports it as an error. |
@znfgnu Thanks! For linter I think in this case it's reasonable to disable them. You can add inline comment to avoid the checking. For example if it is Pylint then you will do something like # pylint: disable=xxx, you can look up the syntax from the repository. |
@noklam I think it's ready to review, tests are passing |
package/kedro_viz/launchers/cli.py
Outdated
def viz(host, port, browser, load_file, save_file, pipeline, env, autoreload, params): | ||
"""Visualise a Kedro pipeline using Kedro viz.""" | ||
from kedro_viz import server |
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.
Is there a specific reason to import server module instead of
from kedro_viz.server import run_server
?
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.
Just to fulfill requirements of linter: there are two functions imported from this module: run_server
and is_localhost
. In case of importing them separately, linter tests report error that there are more than 15 local variables (16). Importing whole server
fulfills this requirement and doesn't require to bypass checks.
I'm pretty convinced that in case of performance it doesn't make a big difference. In case of code style - I leave this decision to maintainers. My the only one intent was to pass linter tests without bypassing them.
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 am not sure why this increases the number of variables, as I understand the changes are moved 2 of them to kedro.constants
and moving some of the import from the top level to the function lazy imports. So the total number of local imports should be unchanged.
I would prefer to keep the top-level module and suppress the listings in this case.
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 am not sure why this increases the number of variables, as I understand the changes are moved 2 of them to
kedro.constants
and moving some of the import from the top level to the function lazy imports. So the total number of local imports should be unchanged.
When you import names inside the function, they are being put in locals()
dictionary, as opposed to top-level imports when names are being put into globals()
.
Number of local imports differ between from kedro_viz.server import is_localhost, run_server
and from kedro_viz import server
because in the first case you import two names (is_localhost
and run_server
) and in the second case only one (server
) is imported.
I would prefer to keep the top-level module and suppress the listings in this case.
What exactly do you mean by keeping the top-level module?
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.
Thank you for explaining! I thought it is complaining about the imported objects instead of the function locals.
I mean this keeping this from kedro_viz.server import run_server
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'll change this in the evening
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.
Done
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.
Thanks! This is a nice small change as I often need to uninstall viz for development purposes! I dropped a small comment but it works nicely, I tested it on my Windows machine 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.
Thank you for your contribution ⭐️ @znfgnu
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.
Thanks so much for this @znfgnu!
Can you please add a line to the release notes file?
No problem, it was fun to find and fix this bottleneck :) As I started using kedro recently, this problem used to affect my work too ;)
Sure, I'll do it in the evening. |
- server.py: Moved `DEFAULT_HOST` and `DEFAULT_PORT` to `constants.py` - launchers/cli.py: Moved `from kedro_viz.server ...` statement to viz function - modified tests so they follow new code structure Signed-off-by: Konrad Sikorski <znfgnu@gmail.com>
DEFAULT_HOST
andDEFAULT_PORT
toconstants.py
from kedro_viz.server ...
statement to viz functionSigned-off-by: Konrad Sikorski znfgnu@gmail.com
Description
Some
kedro-viz
users experience very long startup time ofkedro
CLI script right after installingkedro-viz
.On my computer (Intel® Core™ i7-5600U CPU @ 2.60GHz × 4 w/ some SSD drive), running
kedro
command took:kedro-viz
installed: 0,61-0,75s (10 tries)kedro-viz
installed: 2,39-3,76s (10 tries)On slower machines the problem is even more disturbing - I experienced almost ~10s delay each kedro execution.
Every time one runs the pipeline via kedro CLI, kedro_viz and its dependencies are imported, even if they're not used.
Time measurements
After small changes I made it's 0,67-0,79s, preserving usability.
Time measurements after rearranging imports
The problem was also noted in kedro-org/kedro#1476
Checklist
RELEASE.md
file