Skip to content
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

lazy import modules to reduce importing time #4802

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DanielZhangQD
Copy link
Contributor

@DanielZhangQD DanielZhangQD commented Feb 23, 2025

Part of #4678
This PR only covers the third-party modules to import lazily, however, from the test result below, it still takes a long time to import the whole sky module.
So this needs to be improved further, e.g.:

  • Lazy import of other unnecessary modules
  • Split some core modules further to make the import more fine-grained

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please check the separate comments below)
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

@DanielZhangQD
Copy link
Contributor Author

Method to verify the import time:
Execute the scripts with command:

./run_clean_import.sh --force-reinstall

Result of this change:

  • MacOS with arm: total time from 5.258s to 2.053s
    • Master
image
  • This PR
image
  • Linux: total time from 1.595s to 0.916s
    • Master
image
  • This PR
image

@DanielZhangQD
Copy link
Contributor Author

Hi @Michaelvll, could you please help trigger smoke_test for this PR? Thanks!

BTW, as I have described in the PR description, this PR only covers the 3rd party modules and the import time is still long and needs to be reduced further.
However, further improvement may need to touch the business logic of the core modules, e.g. split some complicated modules, and I prefer to do that in separate PRs later, what do you think?

@concretevitamin
Copy link
Member

/quicktest-core

@Michaelvll
Copy link
Collaborator

Thanks for submitting this PR @DanielZhangQD! Doing the core part inanother PR sounds good to me.

I have some quick questions:

  1. The main goal of doing this is to speed up some of our core operations. Does making this lazy import help: (a) sky status, (b) sky launch, (c) sky exec, (d) sky queue? Since some of those operations involves ssh and run inline python script that imports sky, reducing the import time may help those case. It would be better to have some profiling for this.
  2. Is there any drawback for making those third-party lazy import?

@DanielZhangQD
Copy link
Contributor Author

Hi @Michaelvll, thanks for the info!
I will:

  1. Profile those core operations
  2. Yes, there may be some potential issues for the lazy import, e.g.:
  • 2.1 Race condition in multithread importing
  • 2.2 Potential broken type hints
  • 2.3 Missing or incompatible dependencies are only detected at runtime when the lazy import is triggered, not during startup
  • 2.4 The first use of a lazily imported module incurs import overhead
  • ...

For 2.1, I see Class LazyImport already includes a lock to avoid this issue.
For the others, we must thoroughly test this PR to avoid functionality breaks and balance the benefits v.s. cost

And I will fix the issue with quicktest-core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants