-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
[CI] Speed up wheel commit validation check by 100x #33434
Conversation
…s been creeping through many of ci/cd builds in our pipeline. The existing code uses pipe to read from a rather large file (>50MB). Pipe however has buffer limit which by default in term of kb (https://man7.org/linux/man-pages/man7/pipe.7.html) so what we look for might not exist. We can fix this by tell unzip the exact file we are looking for. That file is pretty small so we should not hit buffer limit. You might notice other surpises might still happen with this fix (e.g. many files that match ^__commit__). This sanity check goes back to 2 years ago by our veteran Kai (234b015) to sanity check issues with stale artifacts from previous builds or race conditions between builds. Further investigation on how builkite agent multi-tenant is setup might or might not simplify this logic further. Signed-off-by: Cuong Nguyen <can@anyscale.com>
linux pipe is a streaming setup, right? why the buffer size matters? |
are you able to reproduce the issue and find out the root cause? |
@aslonnie good point, and you're absolutely correct, the stdin is stream; purely a theory but my best guess is streaming such a large stdin makes buffer and grep unpredictable, and hopefully no harm to reduce the data usage a bit I can find and download the problematic wheel (e.g. see artifacts and download cp39 in https://buildkite.com/ray-project/oss-ci-build-branch/builds/2791#0186ee52-dbab-4169-839c-f8eab80791be), but running the same grep command locally returns a proper commit, so the wheel is ok, but the empty commit return is not reproducible
The second command is also much (100x) faster |
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'm sus of the hypothesis that large files would cause grep to fail. But the speed up is great 😀 maybe change PR title to something like "speed up commit extraction from wheel"?
ci/ci.sh
Outdated
@@ -415,7 +415,7 @@ validate_wheels_commit_str() { | |||
continue | |||
fi | |||
|
|||
WHL_COMMIT=$(unzip -p "$whl" | grep "^__commit__" | awk -F'"' '{print $2}') | |||
WHL_COMMIT=$(unzip -p "$whl" "*ray/__init__.py" | grep "^__commit__" | awk -F'"' '{print $2}') | |||
|
|||
if [ "${WHL_COMMIT}" != "${EXPECTED_COMMIT}" ]; then | |||
echo "Error: Observed wheel commit (${WHL_COMMIT}) is not expected commit (${EXPECTED_COMMIT}). Aborting." |
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.
Can we add a log to print which wheel file it came from?
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.
Absolutely!
Signed-off-by: Cuong Nguyen <can@anyscale.com>
cc @jjyao |
w00h00 thank you both! |
Speed up wheel commit validation check by 100x. Also hopefully will alleviate if not eliminate the 'Observed wheel commit () is not expected' issue (ray-project#32156) that has been creeping through many of ci/cd builds in our pipeline. The existing code uses pipe to read from a rather large file (>50MB). Pipe however has buffer limit which by default in term of kb (https://man7.org/linux/man-pages/man7/pipe.7.html) so what we look for might not exist. We can fix this by tell unzip the exact file we are looking for. That file is pretty small so we should not hit buffer limit. Signed-off-by: Cuong Nguyen <can@anyscale.com> Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
Speed up wheel commit validation check by 100x. Also hopefully will alleviate if not eliminate the 'Observed wheel commit () is not expected' issue (ray-project#32156) that has been creeping through many of ci/cd builds in our pipeline. The existing code uses pipe to read from a rather large file (>50MB). Pipe however has buffer limit which by default in term of kb (https://man7.org/linux/man-pages/man7/pipe.7.html) so what we look for might not exist. We can fix this by tell unzip the exact file we are looking for. That file is pretty small so we should not hit buffer limit. Signed-off-by: Cuong Nguyen <can@anyscale.com>
Speed up wheel commit validation check by 100x. Also hopefully will alleviate if not eliminate the 'Observed wheel commit () is not expected' issue (ray-project#32156) that has been creeping through many of ci/cd builds in our pipeline. The existing code uses pipe to read from a rather large file (>50MB). Pipe however has buffer limit which by default in term of kb (https://man7.org/linux/man-pages/man7/pipe.7.html) so what we look for might not exist. We can fix this by tell unzip the exact file we are looking for. That file is pretty small so we should not hit buffer limit. Signed-off-by: Cuong Nguyen <can@anyscale.com> Signed-off-by: elliottower <elliot@elliottower.com>
Speed up wheel commit validation check by 100x. Also hopefully will alleviate if not eliminate the 'Observed wheel commit () is not expected' issue (ray-project#32156) that has been creeping through many of ci/cd builds in our pipeline. The existing code uses pipe to read from a rather large file (>50MB). Pipe however has buffer limit which by default in term of kb (https://man7.org/linux/man-pages/man7/pipe.7.html) so what we look for might not exist. We can fix this by tell unzip the exact file we are looking for. That file is pretty small so we should not hit buffer limit. Signed-off-by: Cuong Nguyen <can@anyscale.com> Signed-off-by: Jack He <jackhe2345@gmail.com>
Why are these changes needed?
Speed up wheel commit validation check by 100x. Also hopefully will alleviate if not eliminate the 'Observed wheel commit () is not expected' issue (#32156) that has been creeping through many of ci/cd builds in our pipeline.
The existing code uses pipe to read from a rather large file (>50MB). Pipe however has buffer limit which by default in term of kb (https://man7.org/linux/man-pages/man7/pipe.7.html) so what we look for might not exist. We can fix this by tell unzip the exact file we are looking for. That file is pretty small so we should not hit buffer limit.
You might notice other surpises might still happen with this fix (e.g. many files that match ^commit). This sanity check goes back to 2 years ago by our veteran Kai (234b015) to sanity check issues with stale artifacts from previous builds or race conditions between builds. Further investigation on how builkite agent multi-tenant is setup might or might not simplify this logic further.
Signed-off-by: Cuong Nguyen can@anyscale.com
@can-anyscale
Related issue number
#32156
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.