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

[BugFix] Fix modification of sky/__init__.py when building sky wheels #2733

Merged
merged 4 commits into from
Oct 23, 2023

Conversation

cblmemo
Copy link
Collaborator

@cblmemo cblmemo commented Oct 23, 2023

To reproduce:

python -c 'from sky.backends import wheel_utils; wheel_utils.build_sky_wheel()'

This will replace the _SKYPILOT_COMMIT_SHA in sky/__init__.py with the current commit. This PR fixed the problem.
image

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify 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: bash tests/backward_comaptibility_tests.sh

@cblmemo cblmemo changed the title [BugFix] Fix changing of sky/__init__.py when building sky wheels [BugFix] Fix modification of sky/__init__.py when building sky wheels Oct 23, 2023
@Michaelvll
Copy link
Collaborator

Ahh, nice catch @cblmemo! I was trying to only modify the __init__ file in the tmpdir, to avoid race condition for multiple builds but it seems we are doing symlink for the folder, so it will reflect to the original skypilot codebase. Is there a way we can only update the __init__ file, e.g., we symlink all the other files/folders, but copy the __init__.py and change it?

@cblmemo
Copy link
Collaborator Author

cblmemo commented Oct 23, 2023

Ahh, nice catch @cblmemo! I was trying to only modify the __init__ file in the tmpdir, to avoid race condition for multiple builds but it seems we are doing symlink for the folder, so it will reflect to the original skypilot codebase. Is there a way we can only update the __init__ file, e.g., we symlink all the other files/folders, but copy the __init__.py and change it?

Done 🫡

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix @cblmemo! Could we quickly test the speed for building the wheel with and without this PR? I suppose there won't be much difference. : )

sky/backends/wheel_utils.py Outdated Show resolved Hide resolved
sky/backends/wheel_utils.py Outdated Show resolved Hide resolved
cblmemo and others added 2 commits October 23, 2023 16:29
Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com>
Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com>
@cblmemo
Copy link
Collaborator Author

cblmemo commented Oct 23, 2023

Thanks for the quick fix @cblmemo! Could we quickly test the speed for building the wheel with and without this PR? I suppose there won't be much difference. : )

It even became faster 🤣 Though it should be some irrelevant reasons

w/ this PR: Average at 2.029663157463074

Original data: 10 times
[2.0189316272735596, 1.9405455589294434, 2.0807836055755615, 2.0257434844970703, 2.1302366256713867, 2.126328945159912, 1.9333207607269287, 2.0329902172088623, 1.971566915512085, 2.0361838340759277]

w/o this PR: Average at 2.0818066596984863

Original data: 10 times
[2.3937103748321533, 1.940063238143921, 1.9440243244171143, 2.032553195953369, 2.0736310482025146, 2.0780720710754395, 2.1743052005767822, 2.1686527729034424, 2.0339012145996094, 1.9791531562805176]

Test script:

from sky.backends import wheel_utils
import shutil,time

ts = []

for _ in range(10):
    st = time.time()
    shutil.rmtree(wheel_utils.WHEEL_DIR, ignore_errors=True)
    wheel_utils.build_sky_wheel()
    ed = time.time()
    ts.append(ed - st)


print(ts)
print(sum(ts) / len(ts))

@cblmemo
Copy link
Collaborator Author

cblmemo commented Oct 23, 2023

Just found out that I left the shutil.rmtree inside the timer... Now it looks like the following and the result is the same as the previous one.

from sky.backends import wheel_utils
import shutil, time

ts = []

for _ in range(10):
    shutil.rmtree(wheel_utils.WHEEL_DIR, ignore_errors=True)
    st = time.time()
    wheel_utils.build_sky_wheel()
    ed = time.time()
    ts.append(ed - st)


print(ts)
print(sum(ts) / len(ts))

w/ this PR: Average at 2.0500237226486204

Original data: 10 times
[2.0702223777770996, 2.0205273628234863, 2.078216552734375, 2.1142842769622803, 1.9742474555969238, 2.0165488719940186, 1.971510648727417, 2.0825717449188232, 2.09531569480896, 2.0767922401428223]

w/o this PR: Average at 2.0342708349227907

Original data: 10 times
[2.1428840160369873, 2.0071356296539307, 1.9189136028289795, 2.035106897354126, 2.030364513397217, 2.0747296810150146, 2.0613133907318115, 1.944849967956543, 2.071007013320923, 2.056403636932373]

@cblmemo cblmemo merged commit 048f45a into master Oct 23, 2023
18 checks passed
@cblmemo cblmemo deleted the fix-wheel-build branch October 23, 2023 23:47
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.

None yet

2 participants