[CI][Temp fix] make threshold a soft fail for multiprocessing test#2661
[CI][Temp fix] make threshold a soft fail for multiprocessing test#2661KuntaiDu merged 2 commits intoLMCache:devfrom
Conversation
Signed-off-by: ApostaC <yihua98@uchicago.edu>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adjusts the behavior of the multiprocessing performance test in the CI pipeline. The primary goal is to prevent the entire CI run from failing due to performance regressions that are within a tolerable range, instead issuing a warning. This change allows for better visibility into performance trends without blocking development, treating threshold breaches as warnings rather than hard failures. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request modifies a CI script to make a performance threshold check a soft fail instead of a hard fail. The implementation introduces a global variable in the embedded Python script to track failure status. While the change achieves its goal, using a global variable and mixing failure detection logic with formatting logic in the format_comparison function reduces code quality. My review includes a suggestion to refactor the Python code to separate these concerns for better maintainability and clarity.
| def format_comparison(name, lmcache_val, baseline_val): | ||
| global failed |
There was a problem hiding this comment.
Using global variables introduces side effects, making functions like format_comparison harder to understand, test, and reuse. This change also mixes the logic for checking failure conditions with the logic for formatting a string, which is a violation of the Single Responsibility Principle.
A cleaner approach would be to separate these concerns. You could have a dedicated loop to determine if the failure threshold is met, and keep format_comparison purely for formatting. For example:
# In the main script body, before printing results
failed = False
for name, baseline_val, lmcache_val in metrics:
baseline = safe_float(baseline_val)
lmcache = safe_float(lmcache_val)
if baseline is not None and lmcache is not None and baseline > 0:
diff_pct = ((lmcache - baseline) / baseline) * 100
if diff_pct > 10:
failed = True
break # Found a failure, no need to check more
# ... then call printing functions ...
# Then at the end of the script
if failed:
# ...With this approach, format_comparison would not need to be modified to handle the failed flag at all, and the global variable would be handled at the script's top level, which is more appropriate.
…MCache#2661) [temp fix] make threshhold soft fail for mp ci Signed-off-by: ApostaC <yihua98@uchicago.edu>
…MCache#2661) [temp fix] make threshhold soft fail for mp ci Signed-off-by: ApostaC <yihua98@uchicago.edu> Signed-off-by: Ofer Kiselov Nahman <ofer.kiselovnahman@weka.io>
…MCache#2661) [temp fix] make threshhold soft fail for mp ci Signed-off-by: ApostaC <yihua98@uchicago.edu>
…MCache#2661) [temp fix] make threshhold soft fail for mp ci Signed-off-by: ApostaC <yihua98@uchicago.edu>
…MCache#2661) [temp fix] make threshhold soft fail for mp ci Signed-off-by: ApostaC <yihua98@uchicago.edu> Signed-off-by: shaoxiawjc <wjc2800@163.com>
…MCache#2661) [temp fix] make threshhold soft fail for mp ci Signed-off-by: ApostaC <yihua98@uchicago.edu> Signed-off-by: Aaron Wu <aaron.wu@dell.com>
…MCache#2661) [temp fix] make threshhold soft fail for mp ci Signed-off-by: ApostaC <yihua98@uchicago.edu>
…MCache#2661) [temp fix] make threshhold soft fail for mp ci Signed-off-by: ApostaC <yihua98@uchicago.edu>
What this PR does / why we need it:
Special notes for your reviewers:
If applicable: