Skip to content

Optimize import speed for Mars package #3022

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
May 13, 2022
Merged

Optimize import speed for Mars package #3022

merged 10 commits into from
May 13, 2022

Conversation

@wjsi
Copy link
Member

@wjsi wjsi commented May 11, 2022 •
edited
Loading

What do these changes do?

Related issue number

Fixes #3020

Check code requirements

  • tests added / passed (if needed)
  • Ensure all linting tests pass, see here for how to run them

@wjsi wjsi added type: enhancement request mod: deploy to be backported Indicate that the PR need to be backported to stable branch labels May 11, 2022
@wjsi wjsi added this to the v0.10.0a1 milestone May 11, 2022
@wjsi wjsi force-pushed the enh/opt_import branch 13 times, most recently from e95e065 to c2907d6 Compare May 12, 2022 05:15
@wjsi wjsi marked this pull request as ready for review May 12, 2022 07:06
@chaokunyang
Copy link
Contributor

chaokunyang commented May 12, 2022

Great work, with this optimization, the import time has reduced from 5s to 2s.
image
I uploaded the new import to see whether there are other places we can optimize:
import.log

chaokunyang
chaokunyang reviewed May 12, 2022
View reviewed changes
@wjsi
Copy link
Member Author

wjsi commented May 12, 2022

Great work, with this optimization, the import time has reduced from 5s to 2s. image I uploaded the new import to see whether there are other places we can optimize: import.log

For this result, seems that vineyard is importing kubernetes and pkg_resources, contributing to a long load time. Will fix right now.

@wjsi wjsi force-pushed the enh/opt_import branch from 02bbb42 to 60c6f91 Compare May 12, 2022 13:53
@wjsi wjsi requested a review from sighingnow as a code owner May 12, 2022 13:53
@wjsi wjsi force-pushed the enh/opt_import branch from 60c6f91 to 032a0cb Compare May 12, 2022 14:00
sighingnow
sighingnow previously approved these changes May 13, 2022
View reviewed changes
Copy link
Contributor

@sighingnow sighingnow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

It's ok to import vineyard lazily, thanks for addressing that.

@qinxuye
Copy link
Collaborator

qinxuye commented May 13, 2022

Any further optimization? @chaokunyang @wjsi

@chaokunyang
Copy link
Contributor

chaokunyang commented May 13, 2022

Tooks 1.6s now, any chance that to limit it to 1 second?
image
import.log

@wjsi
Copy link
Member Author

wjsi commented May 13, 2022 •
edited
Loading

Tooks 1.6s now, any chance that to limit it to 1 second? image import.log

Seems vineyard is still importing via mars.storage.vineyard. Make sure if you have updated to the latest commit.

Given my tests, after vineyard is installed, it is not imported now. As the majority of time cost is brought by numpy and pandas imports, we'd better stop here as further changes will make Mars code hard to maintain.

fyrestone
fyrestone reviewed May 13, 2022
View reviewed changes
mars/services/task/execution/ray/executor.py Outdated
input_keys: List[str],
*inputs,
):
from .....optimization.physical import optimize
Copy link
Contributor

@fyrestone fyrestone May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import inside the function makes it runs slow. Also it's better to import at top of the file: https://peps.python.org/pep-0008/#imports

Copy link
Member Author

@wjsi wjsi May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When sticking to fast import we have no choices but make compromises. @chaokunyang any ideas?

@wjsi wjsi force-pushed the enh/opt_import branch from 032a0cb to a3b24cd Compare May 13, 2022 05:18
@wjsi wjsi requested a review from a team as a code owner May 13, 2022 06:47
@wjsi wjsi force-pushed the enh/opt_import branch from 199582a to 47525ac Compare May 13, 2022 06:59
@chaokunyang
Copy link
Contributor

chaokunyang commented May 13, 2022

image
import.log

chaokunyang
chaokunyang approved these changes May 13, 2022
View reviewed changes
Copy link
Contributor

@chaokunyang chaokunyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems there are still improvement space for mars init, but I think we can merge this first, and leave further optimization in laster pr

qinxuye
qinxuye reviewed May 13, 2022
View reviewed changes
Copy link
Collaborator

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

qinxuye
qinxuye approved these changes May 13, 2022
View reviewed changes
Copy link
Collaborator

@qinxuye qinxuye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wjsi wjsi merged commit acecc9c into mars-project:master May 13, 2022
@wjsi wjsi deleted the enh/opt_import branch May 13, 2022 07:53
wjsi added a commit to wjsi/mars that referenced this pull request May 13, 2022
@wjsi
Optimize import speed for Mars package (mars-project#3022)
f4671bf
@wjsi wjsi removed the to be backported Indicate that the PR need to be backported to stable branch label May 13, 2022
wjsi added a commit to wjsi/mars that referenced this pull request May 13, 2022
@wjsi
Optimize import speed for Mars package (mars-project#3022)
60cd34b
@wjsi wjsi mentioned this pull request May 13, 2022
Merged
wjsi added a commit that referenced this pull request May 13, 2022
@wjsi
[BACKPORT] Optimize import speed for Mars package (#3022) (#3035)
3929ff9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fyrestone fyrestone fyrestone left review comments

@qinxuye qinxuye qinxuye approved these changes

@chaokunyang chaokunyang chaokunyang approved these changes

@sighingnow sighingnow sighingnow left review comments

@hekaisheng hekaisheng Awaiting requested review from hekaisheng

Assignees

No one assigned

Labels

mod: deploy type: enhancement request

Projects

None yet

Milestone

v0.10.0a1

Development

Successfully merging this pull request may close these issues.

[BUG] Mars import took too long

5 participants

@wjsi @chaokunyang @qinxuye @fyrestone @sighingnow