Fix regex in benchmarks cpu/disk_on_idle (BugFix)#2487
Conversation
* for disk_on_idle and cpu_on_idle, fix regex to resolve warning about invalid escape char in newer python3 versions. (older Python3 just silently passed it) * Also added CLAUDE.md to gitignore to avoid accidentally pulling in Claude instructions when making commits. Fixes #2477
There was a problem hiding this comment.
Pull request overview
Fixes Python SyntaxWarning noise in benchmark jobs by making the regex patterns raw strings (so \S is treated as a regex escape rather than an invalid Python string escape), and prevents accidental commits of local Claude instruction files.
Changes:
- Update
benchmarks/system/cpu_on_idleandbenchmarks/system/disk_on_idlecommands to use raw-string regex patterns inre.findall(...). - Add
CLAUDE.mdto.gitignore.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
providers/base/units/benchmarks/jobs.pxu |
Uses raw-string regex literals in two benchmark job commands to eliminate invalid-escape warnings on newer Python versions. |
.gitignore |
Ignores CLAUDE.md to avoid committing local tooling instructions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2487 +/- ##
==========================================
+ Coverage 58.91% 58.93% +0.01%
==========================================
Files 476 477 +1
Lines 48011 48034 +23
Branches 8569 8571 +2
==========================================
+ Hits 28286 28309 +23
+ Misses 18833 18830 -3
- Partials 892 895 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…pt, add unit tests, and update the job descriptions accordingly
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fernando79513
left a comment
There was a problem hiding this comment.
First of all, thanks a lot for taking the time to translate this test to Python. I think it is much clearer what the test is doing now.
I am still not sure what the purpose of running this test is. AFAIK, it's just informative.
I have a couple of concerns with the previous implementation:
- The "parse_iostat_column" function only seems to work because we are choosing to parse the last value of the row. If we wanted a different column, we would have to rewrite the functionality. At this point is just a "get_idle_value" or "get_util_value"
- What happens if there is more than one disk on the device? How do we know which disk to check? I tested it locally and I got something like this:
Device r/s rMB/s rrqm/s %rrqm r_await rareq-sz w/s wMB/s wrqm/s %wrqm w_await wareq-sz d/s dMB/s drqm/s %drqm d_await dareq-sz f/s f_await aqu-sz %util
nvme0n1 42.76 2.47 4.08 8.71 0.12 59.09 19.88 1.41 23.14 53.79 3.09 72.61 0.00 0.00 0.00 0.00 0.00 0.00 0.72 0.35 0.07 0.79
sda 0.00 0.00 0.00 2.16 1.34 23.25 0.00 0.00 0.00 16.67 5.80 4.80 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00
sdb 0.00 0.00 0.00 51.93 2.70 11.19 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00
| except subprocess.CalledProcessError as e: | ||
| print("ERROR: iostat failed: {}".format(e), file=sys.stderr) | ||
| return 1 |
There was a problem hiding this comment.
No need to try/except if you are going to raise the error anyway.
|
|
||
| CLAUDE.md |
There was a problem hiding this comment.
There is already a CLAUDE.md file in checkbox
| CLAUDE.md |
|
|
||
|
|
||
| def parse_iostat_column(output, column): | ||
| values = [float(n) for n in re.findall(column + r"\n.*?(\S+)\n", output)] |
There was a problem hiding this comment.
I know this regex was here before, but it's only working because the value you are checking is the last one of the table:
avg-cpu: %user %nice %system %iowait %steal %idle
3.62 0.00 2.46 0.08 0.00 93.84
Also, what happens if there is more than one disk?
You are getting only the value of the first disk displayed on the list:
Device r/s rMB/s rrqm/s %rrqm r_await rareq-sz w/s wMB/s wrqm/s %wrqm w_await wareq-sz d/s dMB/s drqm/s %drqm d_await dareq-sz f/s f_await aqu-sz %util
sda 0.00 0.00 0.00 0.00 0.00 0.00 0.50 0.00 0.25 33.33 4.00 8.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.20
sdb 0.00 0.00 0.00 0.00 0.00 0.00 0.50 0.00 0.25 33.33 4.00 8.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.00 0.21
Is that the intended behavior?
| stderr=subprocess.PIPE, | ||
| universal_newlines=True, | ||
| check=True, | ||
| ) |
There was a problem hiding this comment.
If you are analyzing the output, it's simpler to use subprocess.check_output
| ) | |
| result = subprocess.check_output( | |
| ["iostat", "-x", "-m", "1", str(args.time)], | |
| universal_newlines=True, | |
| ) |
| print( | ||
| "ERROR: No '{}' values found in iostat output".format(column), | ||
| file=sys.stderr, | ||
| ) | ||
| return 1 |
There was a problem hiding this comment.
Just raise a system exit. Don't print and return 1.
| ) | ||
| return 1 | ||
| print("{:.2f}%".format(sum(values) / len(values))) | ||
| return 0 |
There was a problem hiding this comment.
No need to return 0
Description
Resolved issues
Resolves #2477
Resolves CHECKBOX-2238
Documentation
NA
Tests
Tested on a fresh Bionic install to verify rawstrings does work back that far:
For comparison, this is Resolute where the initial warnings appeared for me, showing before and after: