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

Use Python for sccache hit rate computation. #2474

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Oct 21, 2024

Fixes an issue in CI computations of sccache hit rates. See rapidsai/cuvs#414 for details.

@bdice bdice requested a review from a team as a code owner October 21, 2024 19:16
@bdice bdice requested a review from KyleFromNVIDIA October 21, 2024 19:16
@bdice bdice added bug Something isn't working non-breaking Non-breaking change labels Oct 21, 2024
@bdice bdice self-assigned this Oct 21, 2024
@AyodeAwe AyodeAwe merged commit 714e07b into rapidsai:branch-24.12 Oct 21, 2024
27 checks passed
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA left a comment

Choose a reason for hiding this comment

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

One small suggestion, you can take it or leave it. I don't need to review again.

@@ -463,14 +463,14 @@ if (( ${NUMARGS} == 0 )) || hasArg libraft || hasArg docs || hasArg tests || has
if [[ ${CACHE_TOOL} == "sccache" && -x "$(command -v sccache)" ]]; then
COMPILE_REQUESTS=$(sccache -s | grep "Compile requests \+ [0-9]\+$" | awk '{ print $NF }')
CACHE_HITS=$(sccache -s | grep "Cache hits \+ [0-9]\+$" | awk '{ print $NF }')
HIT_RATE=$(echo - | awk "{printf \"%.2f\n\", $CACHE_HITS / $COMPILE_REQUESTS * 100}")
HIT_RATE=$(python3 -c "print(f'{${CACHE_HITS} / ${COMPILE_REQUESTS}:.2f}' if ${COMPILE_REQUESTS} else 'nan')")
Copy link
Contributor

Choose a reason for hiding this comment

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

For situations like this, I prefer to pass variables in as arguments, environment variables, etc. to avoid the risk of code injection. It's admittedly not a big deal in this particular situation, but I still prefer to do it.

Suggested change
HIT_RATE=$(python3 -c "print(f'{${CACHE_HITS} / ${COMPILE_REQUESTS}:.2f}' if ${COMPILE_REQUESTS} else 'nan')")
HIT_RATE=$(COMPILE_REQUESTS="${COMPILE_REQUESTS}" CACHE_HITS="${CACHE_HITS}" python3 -c "import os; print(f'{int(os.getenv(\"CACHE_HITS\")) / int(os.getenv(\"COMPILE_REQUESTS\")):.2f}' if int(os.getenv(\"COMPILE_REQUESTS\")) else 'nan')")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build.sh Show resolved Hide resolved
rapids-bot bot pushed a commit to rapidsai/cuvs that referenced this pull request Oct 22, 2024
rapids-bot bot pushed a commit that referenced this pull request Oct 22, 2024
Follow-up PR to address feedback: #2474 (comment)

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

URL: #2475
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants