fix(ci): make import profiler checks warning-only and non-blocking - #17777
fix(ci): make import profiler checks warning-only and non-blocking#17777hebaalazzeh wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes the import profiler checks non-blocking by converting failures into warnings and removing exit code triggers. It also increases the failure threshold in CI from 5000 ms to 20000 ms. The reviewer recommends keeping the threshold at 5000 ms since the check is now warning-only and a 20-second threshold would render the warnings ineffective. Additionally, the reviewer suggests updating the success message to use "threshold" instead of "failure threshold" for consistency with other updated messages.
| python ${PROFILER_SCRIPT} --package ${PACKAGE_NAME} --iterations 11 --fail-threshold 20000 --diff-baseline "${BASELINE_CSV}" --diff-threshold 100 | ||
| else | ||
| python ${PROFILER_SCRIPT} --package ${PACKAGE_NAME} --iterations 11 --fail-threshold 5000 | ||
| python ${PROFILER_SCRIPT} --package ${PACKAGE_NAME} --iterations 11 --fail-threshold 20000 |
There was a problem hiding this comment.
Since the import profiler checks are now non-blocking and warning-only, there is no longer a risk of blocking developer workflows due to threshold violations. Keeping the threshold at a reasonable value (like 5000 ms) is beneficial because it still alerts developers via warnings when imports are slow. Raising the threshold to 20000 ms (20 seconds) is extremely high and will likely prevent any warnings from ever being triggered, defeating the purpose of having a warning-only check.
| python ${PROFILER_SCRIPT} --package ${PACKAGE_NAME} --iterations 11 --fail-threshold 20000 --diff-baseline "${BASELINE_CSV}" --diff-threshold 100 | |
| else | |
| python ${PROFILER_SCRIPT} --package ${PACKAGE_NAME} --iterations 11 --fail-threshold 5000 | |
| python ${PROFILER_SCRIPT} --package ${PACKAGE_NAME} --iterations 11 --fail-threshold 20000 | |
| python ${PROFILER_SCRIPT} --package ${PACKAGE_NAME} --iterations 11 --fail-threshold 5000 --diff-baseline "${BASELINE_CSV}" --diff-threshold 100 | |
| else | |
| python ${PROFILER_SCRIPT} --package ${PACKAGE_NAME} --iterations 11 --fail-threshold 5000 |
| exit_code = 1 | ||
| final_messages.append(f"WARNING: Median import time ({p50_time:.2f} ms) exceeds the threshold ({fail_threshold} ms).") | ||
| else: | ||
| final_messages.append(f"SUCCESS: Median import time ({p50_time:.2f} ms) is within the failure threshold ({fail_threshold} ms).") |
There was a problem hiding this comment.
For consistency with the other updated messages (which now refer to "threshold" instead of "failure threshold"), we should update this success message to also use "threshold".
| final_messages.append(f"SUCCESS: Median import time ({p50_time:.2f} ms) is within the failure threshold ({fail_threshold} ms).") | |
| final_messages.append(f"SUCCESS: Median import time ({p50_time:.2f} ms) is within the threshold ({fail_threshold} ms).") |
|
Can we keep them as blocking but increase the threshold? If there are only warnings, we may not catch regressions (assuming the CI is green) |
2 participants
This PR updates the import profiler regression checks to be non-blocking. Instead of triggering a hard failure, threshold violations will output warnings so GHA checks remain green and do not block developer workflows.
Key Changes
exit_code = 1assignments when thresholds are exceeded. The script will now log violations as aWARNINGand exit with0.ci/run_single_test.shfrom5000msto20000ms(20 seconds).profiler.pyto remove misleading references to "bypassing failure" since failures are no longer triggered.