Error-aware benchmarking: error gate, refine/efficiency modes, health check - #12
Open
SerpentXSF wants to merge 16 commits into
Open
Error-aware benchmarking: error gate, refine/efficiency modes, health check#12SerpentXSF wants to merge 16 commits into
SerpentXSF wants to merge 16 commits into
Conversation
…eilings) Measure ASIC error rate per voltage/frequency combo and select the most efficient setting that stays under a configurable ceiling, instead of the raw fastest. The upstream tool never looks at the hardware error rate, so on BM1370 boards it can pick an aggressive undervolt that hashes fast but throws double-digit errors. - Error metric: mean errorPercentage over the stable window (errorPercentage is a rolling rate, not a cumulative ratio), plus a raw errorCount delta as a comparable diagnostic. - Error gate -> efficiency selection with --max-error (default 3.5%); falls back to lowest-error when nothing clears the ceiling. - refine mode: hold frequency, sweep voltage only, stop at the lowest voltage under the ceiling. Fast single-ASIC rescue path. - Configurable --max-temp (default 66; Gamma boards often need 68), --resume, --benchmark-time, CSV output and a ranked summary table. - Skip transient post-reboot sensor readings instead of aborting a combo; guard outlier trims against short sample counts. - Wrap the run in main(); add unit tests for the decision logic. Backward compatible: existing invocations behave as before, now also reporting the error rate. Fork of mrv777/Bitaxe-Hashrate-Benchmark.
… pass) Fixes from a code review of the error-aware fork, validated on hardware — refine autonomously walked a thermally-boxed Gamma from ~9% to 3.6% error by dropping frequency once voltage hit the temperature ceiling. Crash / accuracy: - Fix ZERO_HASHRATE returning a 7-tuple that crashed the caller's 8-value unpack; add a regression test covering every iteration return path. - Restore the device only once on exit: drop the redundant reboot in reset_to_best_setting and let the finally block own restoration on error. - Average power over the same post-warmup window as hashrate/error so J/TH comparisons across combos are apples-to-apples. - window_error_count sums positive deltas, so a mid-window counter reset no longer yields a bogus value. Smarter tuning: - refine drops frequency and retries when a frequency is thermally boxed in (temp ceiling reached before the error clears), then probes downward for a leaner passer once one is found. - Abort a combo early when its best-case mean can no longer reach the ceiling. Robustness: - Verify settings were actually applied (read back voltage/frequency, retry once) before trusting a combo's measurements. - --resume globs the latest results file so it survives an hour boundary. - Retry a combo once on a transient info-fetch failure; raise info retries. - Require at least 8 post-warmup samples for a stable error mean. - select_best requires in-tolerance hashrate so a throttling combo can't win. 30 unit tests, all passing; hardware I/O kept thin.
Round two of improvements plus the fixes from a second review pass. New capability: - efficiency mode (--mode efficiency): hold frequency and trim voltage down from the current setting to the leanest voltage that still clears the error ceiling — cuts power and heat on an already-healthy miner with no hashrate loss (refine rescues; efficiency trims). - Early-aborted combos are now recorded as a partial "floor" (earlyAborted), so a marginal device that never clears the ceiling still leaves select_best a fallback instead of reverting blind to defaults. Robustness / correctness: - benchmark_iteration returns a structured result dict instead of a positional tuple, removing the return-arity mismatch class that crashed a run twice. - Grid retreats frequency on a thermal cap instead of ending the sweep, and names the stop reason. - Resume now consults the recorded pass/fail outcome in refine and its downward probe (not just grid), so a resumed run can't climb past a known passer or probe below a known failure. - Early-abort bound drops the max sample to mirror the trimmed-mean metric, so a lone rolling-rate spike can't wrongly abort a passable combo. - Reject non-positive --voltage-step/--frequency-step (would infinite-loop). - efficiency start reports the real reason (apply/network failure) instead of mislabeling it a gate failure, and honors --resume. - Skip the 90s stabilization wait on the final restore (nothing is measured after it), and print a distinct message at the refine frequency floor. Docs/output: - Console shows whether the trimmed mean or plain mean was used. - README and spec updated for the new mode, flags, sample minimum, power window, and trimmed-mean metric. 36 unit tests, all passing.
Three lightweight, dependency-free usability additions: - --check: read-only health snapshot. Measures the current setting once over a short window and reports hashrate / J-TH / error / temp with no voltage or frequency change and no reboot (verified: device uptime keeps climbing). - Up-front expectations before a sweep: per-combo duration, that mining is interrupted and the miner reboots between combos, a rough total for the chosen mode, and the Ctrl+C-restores note. - README "Which mode should I use?" table (check / grid / refine / efficiency) so new users pick the right mode without reading the source. Still stdlib + requests only; core stays a single file. 37 unit tests passing.
Review follow-up: a SIGINT during --check previously fell through to the no-results restore branch, which PATCHed and rebooted the device — breaking the "no changes, no reboot" guarantee (and, in the settings-backfill edge case, could apply values different from what was running). - Add a check_mode flag; handle_sigint now exits immediately on Ctrl+C during a check without touching the device. Covered by a new test. - run_check returns success; a failed check now exits non-zero, and a partial (early-aborted) window is labeled in the output. - Note the shorter ~240s check window in --check help and the README, and suggest --max-temp 68 in the check example (Gammas idle near 66-67C). - Use the _recorded() helper in grid's resume path for consistency. 38 unit tests, all passing.
The fork rewrite dropped the upstream numbered "Benchmarking Process" section without replacing it, and the old version described a single-sweep flow that no longer matches the three modes. Add a current per-mode process explanation: the apply-verify / measure / reduce / select-apply cycle, and how the next-combo decision differs across grid, refine, efficiency, and --check.
Follow-up doc touch-ups from the README accuracy review: - "What this fork adds" now says trimmed mean (matching the metric and the dedicated note), with std/min/max mentioned. - Output field list includes the newer recorded fields: errorRateStd, hashrateWithinTolerance, earlyAborted. - -v/-f help (README + argparse) notes efficiency also defaults to the device's current setting, not just refine.
Reword the README and code comments in a plainer style, drop the heavy em-dash usage, and replace the internal dated design spec with a concise docs/DESIGN.md. No behavior changes; 38 tests still pass.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe benchmark now supports error-aware measurements, gated setting selection, grid/refine/efficiency/check modes, resumable JSON and CSV output, fleet execution, safety restoration, documentation, and hardware-independent tests. ChangesError-aware benchmark workflow
Fleet benchmark helper
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant benchmark_iteration
participant Bitaxe_device
participant monitored_stabilization
participant result_store
participant selector
CLI->>benchmark_iteration: provide mode and limits
benchmark_iteration->>Bitaxe_device: apply and verify settings
benchmark_iteration->>monitored_stabilization: monitor safety limits
monitored_stabilization->>Bitaxe_device: collect performance and error samples
benchmark_iteration->>result_store: record JSON and CSV result
benchmark_iteration->>selector: evaluate gates and rank results
selector-->>CLI: return selected setting
CLI->>Bitaxe_device: restore settings
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bitaxe_hashrate_benchmark.py`:
- Around line 1127-1138: Update the benchmark mode dispatch around run_refine,
run_efficiency, and run_grid to return or capture a structured outcome that
distinguishes successful stability-limit completion from operational failures
such as APPLY_FAILED and SYSTEM_INFO_FAILURE. Set exit_code to a nonzero value
for failed outcomes even when no exception is raised, while preserving the
existing exception handling and successful completion status.
- Around line 983-987: Update the resume handling in the voltage loop around
already_tested so recorded voltages replay the live branch’s gate and tolerance
checks instead of being unconditionally skipped; stop at the first recorded
failure and only continue downward after a recorded pass. Add a test covering a
resumed efficiency run with a recorded failing voltage.
- Around line 198-201: Update the passers filtering branch around gate_enabled
so error-gate decisions are recomputed from each result’s errorRate and the
active max-error ceiling, rather than reading passedErrorGate; ensure
select_best and combo_passes use this same active-ceiling logic while retaining
passedErrorGate only as output metadata. Add regression coverage for legacy
results missing passedErrorGate and resumed results using a changed ceiling.
- Around line 320-350: Update set_system_settings(), restart_system(), and
reset_to_best_setting() to propagate failure from PATCH/restart requests and
verify the requested coreVoltage/frequency through get_system_info() before
reporting success. Make handle_sigint() and main() mark system_reset_done, save
results, and print restoration-success messages only after restoration succeeds;
preserve failure status and avoid claiming completion while stress-test settings
may remain active.
- Line 11: Update the START_TIME filename component in
bitaxe_hashrate_benchmark.py to include sub-hour precision, such as minutes and
seconds, or another collision-resistant suffix, so repeated runs for the same IP
within an hour produce distinct JSON and CSV filenames.
- Around line 855-861: Update the THERMAL_REASONS branch to leave
current_voltage unchanged and reduce only current_frequency by
frequency_increment; remove the voltage limit check and any voltage-increase
operation while preserving the existing thermal cutoff message and behavior.
In `@README.md`:
- Around line 9-10: The fallback documentation must match select_best() rather
than describing a global lowest-error choice. Update README.md lines 9-10 and
docs/DESIGN.md lines 46-51 to state the complete fallback ordering: hashrate
tolerance, full-window data, error rate, then efficiency; keep the existing
error-gate behavior documented consistently.
- Line 30: Update the restoration documentation to match runtime behavior: in
README.md lines 30 and 143, replace the “or your original” fallback with the
predefined default settings; in docs/DESIGN.md lines 73-78, revise the
restoration contract to state that predefined defaults are applied when no valid
result exists.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6eba7cc6-77fe-47e0-8df5-53dd2edc043c
📒 Files selected for processing (5)
.gitignoreREADME.mdbitaxe_hashrate_benchmark.pydocs/DESIGN.mdtests/test_benchmark_logic.py
…play
- Include minutes/seconds in the results timestamp so two runs for the same IP
within an hour no longer overwrite each other's JSON/CSV.
- Recompute the error gate from errorRate and the active ceiling in select_best
and combo_passes instead of trusting the persisted passedErrorGate, so a
resume with a changed --max-error (or a file predating the field) decides
correctly. Keep passedErrorGate as output metadata only.
- Restore functions return success; on a failed apply/restart the tool warns
that the miner may still be on the last test settings rather than printing a
restore-success message.
- Grid stops on a thermal/power cutoff instead of raising voltage (lower
frequencies at that voltage are already covered; use refine for a
thermally-limited chip).
- Efficiency resume replays the recorded pass/fail branch and stops at the
first recorded failure instead of probing below a known-failing voltage.
- A run that records no results (e.g. settings wouldn't apply) exits non-zero
so automation can detect the failure.
- Docs: describe the full select_best fallback ordering and correct the
restore wording ("starting settings", not "predefined default").
42 unit tests, all passing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/DESIGN.md`:
- Around line 80-82: Update main() and handle_sigint() so their restore calls to
set_system_settings() also validate the returned success value when results is
empty. If restoration returns False, print the same restore-failure warning used
for completed results, while preserving the existing behavior for successful
restores.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 69f42a8f-edc1-4285-8daa-423049adfbea
📒 Files selected for processing (4)
README.mdbitaxe_hashrate_benchmark.pydocs/DESIGN.mdtests/test_benchmark_logic.py
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- bitaxe_hashrate_benchmark.py
Comment on lines
+80
to
+82
| best setting (or the device's starting settings, when nothing was measured) is | ||
| restored on exit and on Ctrl+C, and the tool reports a warning rather than | ||
| claiming success if that restore request fails. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Report a failed restore when no result exists.
When results is empty, main() and handle_sigint() discard the False return from set_system_settings(). A failed PATCH or restart can leave the device on its last test setting without the warning documented here. Check the return value and print the same restore-failure warning in both paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/DESIGN.md` around lines 80 - 82, Update main() and handle_sigint() so
their restore calls to set_system_settings() also validate the returned success
value when results is empty. If restoration returns False, print the same
restore-failure warning used for completed results, while preserving the
existing behavior for successful restores.
Author
|
Thanks for the review. Pushed c3117b8 addressing all eight comments:
On the SSRF warnings for the 42 unit tests pass ( |
The results-empty branches in main() and handle_sigint() discarded the return from set_system_settings(), so a failed restore there would not print the restore-failure warning. Check the return in both paths and share one warning message constant across all four restore sites. Adds a test.
Author
|
Good catch - pushed a follow-up (see latest commit). The results-empty branches in |
…bilization monitoring) Reimplemented natively (not merged) from ideas in upstream PRs mrv777#5/mrv777#8/mrv777#9, plus a couple of small robustness fixes found while testing on hardware: - Hashrate standard deviation per combo (how steady a combo is), recorded and shown alongside the average. - Fan speed telemetry (fanspeed % and fanrpm) per combo, recorded and shown. - Monitor the device during the post-apply stabilization wait: a combo already over the temperature/power limit is dropped there (surfaced as a thermal reason, which refine uses to lower frequency) instead of wasting the window. - Enable ANSI colors on classic Windows terminals (dependency-free os.system("")). - Dockerfile: PYTHONUNBUFFERED=1 for live `docker logs -f` output. - Skip implausible hashrate samples (far above the theoretical maximum) so an unstable chip's garbage readings don't pollute the average or the rankings. - Remove the now-dead skip_wait branch in restart_system; the stabilization wait lives only in monitored_stabilization now. Validated on real BM1370 Gamma hardware. 53 unit tests, all passing. Still stdlib + requests only.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
README.md (3)
31-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude read-only paths from apply and restore claims.
--checkperforms no changes or reboot, and--dry-rundoes not touch the device. Update the “every mode” and generic workflow text to apply only to device-changing benchmark modes.Also applies to: 131-135, 144-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 31, Update the README workflow descriptions around the mode behavior and generic workflow text so claims about applying settings, rebooting, and restoring the best or initial setting explicitly apply only to device-changing benchmark modes. Exclude --check and --dry-run, which must remain described as read-only and non-mutating.
20-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify the backward-compatibility scope.
Line 5 documents changed selection behavior, so “Existing commands work exactly as before” is too broad. State that the existing command syntax remains supported while selection is now error-aware.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 20, Update the README statement around the backward-compatibility claim to specify that existing command syntax remains supported, while selection behavior is now error-aware; remove the broader claim that existing commands work exactly as before.
169-169: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument
errorCountDeltaduration for partial windows.Early-aborted combos store deltas from partial
error_samples, soerrorCountDeltais not directly comparable as a fixed-length window value. Record and document the actual post-warmup duration/sample count for early-aborted results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 169, Document and expose the actual post-warmup duration or sample count for partial windows alongside errorCountDelta in the combo results. Update the early-abort handling that consumes error_samples so it records this value, and revise the README description to clarify that errorCountDelta from aborted combinations covers the recorded partial window rather than a fixed-length interval.
🧹 Nitpick comments (4)
tests/test_benchmark_logic.py (2)
487-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a VR temperature case.
The suite covers the chip temperature limit and the power limit. The
VR_TEMP_EXCEEDEDbranch ofmonitored_stabilizationhas no test.💚 Proposed test
+ def test_aborts_on_over_vr_temp(self): + with mock.patch.object(b, "_quick_info", return_value=make_info(vr=99)), \ + mock.patch.object(b.time, "sleep", return_value=None): + self.assertEqual(b.monitored_stabilization(), "VR_TEMP_EXCEEDED")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_benchmark_logic.py` around lines 487 - 500, Add a test alongside test_aborts_on_over_temp and test_aborts_on_over_power that mocks _quick_info with a VR temperature exceeding its limit, stubs time.sleep, and asserts monitored_stabilization() returns "VR_TEMP_EXCEEDED".
267-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
_runhelper.
test_all_garbage_hashrate_is_no_datarepeats the patch context that_runalready provides.♻️ Proposed change
def test_all_garbage_hashrate_is_no_data(self): - with mock.patch.object(b, "get_system_info", return_value=make_info(hr=99999)), \ - mock.patch.object(b.time, "sleep", return_value=None): - r = b.benchmark_iteration(1150, 525) + r = self._run(make_info(hr=99999)) self.assertFalse(r["ok"]) self.assertEqual(r["reason"], "NO_DATA_COLLECTED")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_benchmark_logic.py` around lines 267 - 272, Update test_all_garbage_hashrate_is_no_data to use the existing _run helper instead of duplicating the get_system_info and time.sleep patch context. Preserve the current benchmark inputs and assertions for the unsuccessful NO_DATA_COLLECTED result.bitaxe_hashrate_benchmark.py (2)
427-434: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse the return value of
set_system_settings.
set_system_settingsnow returns a status. Line 428 discards it. If the PATCH or the restart request fails,apply_settingsstill waits the fullmonitored_stabilization()window before the confirmation GET fails. With two attempts, a device that is unreachable costs up to two complete stabilization windows per combo.Skip the wait when the request already failed.
♻️ Proposed change
for attempt in range(2): - set_system_settings(core_voltage, frequency) + if not set_system_settings(core_voltage, frequency): + print(YELLOW + f"Apply request failed (attempt {attempt + 1}/2); retrying." + RESET) + continue reason = monitored_stabilization()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bitaxe_hashrate_benchmark.py` around lines 427 - 434, Update the retry loop in apply_settings to capture the status returned by set_system_settings and immediately skip to the next attempt when the request fails, avoiding monitored_stabilization for unsuccessful PATCH or restart requests. Preserve the existing stabilization and system-info confirmation flow for successful requests.
1186-1189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
os.system("")with a direct console call.The argument is a constant empty string, so the ast-grep injection hint and Ruff S605/S607 are false positives for exploitability. The call still starts a shell process, and it fails the configured linters. Use the Win32 API directly instead.
♻️ Proposed change
if os.name == "nt": - os.system("") + import ctypes + kernel32 = ctypes.windll.kernel32 + # 7 = STD_OUTPUT_HANDLE | ENABLE_VIRTUAL_TERMINAL_PROCESSING (0x0004) added to the current mode + kernel32.SetConsoleMode(kernel32.GetStdHandle(-11), 7)Confirm that the replacement enables ANSI output on your target Windows versions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bitaxe_hashrate_benchmark.py` around lines 1186 - 1189, Replace the os.system("") call in the Windows branch with a direct Win32 console API call that enables ANSI escape processing, avoiding shell invocation and satisfying the configured linters. Preserve the existing behavior for Windows consoles and verify the API works on the supported target Windows versions.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 11: Update the README descriptions at both the line 11 summary and the
corresponding line 135 section to use the same fallback order: prefer hashrate
within tolerance, then full-window (non-early-aborted) data, then lower error
rate, and finally better efficiency. Remove the conflicting “best of the rest”
or J/TH-first wording while preserving the error-ceiling behavior.
---
Outside diff comments:
In `@README.md`:
- Line 31: Update the README workflow descriptions around the mode behavior and
generic workflow text so claims about applying settings, rebooting, and
restoring the best or initial setting explicitly apply only to device-changing
benchmark modes. Exclude --check and --dry-run, which must remain described as
read-only and non-mutating.
- Line 20: Update the README statement around the backward-compatibility claim
to specify that existing command syntax remains supported, while selection
behavior is now error-aware; remove the broader claim that existing commands
work exactly as before.
- Line 169: Document and expose the actual post-warmup duration or sample count
for partial windows alongside errorCountDelta in the combo results. Update the
early-abort handling that consumes error_samples so it records this value, and
revise the README description to clarify that errorCountDelta from aborted
combinations covers the recorded partial window rather than a fixed-length
interval.
---
Nitpick comments:
In `@bitaxe_hashrate_benchmark.py`:
- Around line 427-434: Update the retry loop in apply_settings to capture the
status returned by set_system_settings and immediately skip to the next attempt
when the request fails, avoiding monitored_stabilization for unsuccessful PATCH
or restart requests. Preserve the existing stabilization and system-info
confirmation flow for successful requests.
- Around line 1186-1189: Replace the os.system("") call in the Windows branch
with a direct Win32 console API call that enables ANSI escape processing,
avoiding shell invocation and satisfying the configured linters. Preserve the
existing behavior for Windows consoles and verify the API works on the supported
target Windows versions.
In `@tests/test_benchmark_logic.py`:
- Around line 487-500: Add a test alongside test_aborts_on_over_temp and
test_aborts_on_over_power that mocks _quick_info with a VR temperature exceeding
its limit, stubs time.sleep, and asserts monitored_stabilization() returns
"VR_TEMP_EXCEEDED".
- Around line 267-272: Update test_all_garbage_hashrate_is_no_data to use the
existing _run helper instead of duplicating the get_system_info and time.sleep
patch context. Preserve the current benchmark inputs and assertions for the
unsuccessful NO_DATA_COLLECTED result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d0f87d2c-cc8a-4902-9ad0-ae3bb8adff8b
📒 Files selected for processing (5)
DockerfileREADME.mdbitaxe_hashrate_benchmark.pydocs/DESIGN.mdtests/test_benchmark_logic.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/DESIGN.md
|
|
||
| - **Error-rate measurement**: a trimmed mean of `errorPercentage` over each combo's stable window (with `std`/`min`/`max` recorded), plus the raw ASIC error count for the window (`errorCountDelta`). | ||
| - **More telemetry per combo**: hashrate standard deviation (how steady the combo is, not just its average), and fan speed (`%` and RPM) — a useful cooling axis alongside temperature. | ||
| - **Error gate, then efficiency**: the best setting is the lowest J/TH among combinations that stay within the error ceiling (`--max-error`, default 3.5%) with hashrate in tolerance, not the raw fastest. When nothing clears the ceiling it falls back to the best of the rest, preferring in-tolerance hashrate and a full (non-early-aborted) window, then lowest error. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use one fallback contract in both sections.
Line 11 and Line 135 describe different fallback behavior. Document the exact implementation order consistently: hashrate tolerance, full-window data, error rate, then efficiency.
Also applies to: 135-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 11, Update the README descriptions at both the line 11
summary and the corresponding line 135 section to use the same fallback order:
prefer hashrate within tolerance, then full-window (non-early-aborted) data,
then lower error rate, and finally better efficiency. Remove the conflicting
“best of the rest” or J/TH-first wording while preserving the error-ceiling
behavior.
A small companion script for running more than one miner. By default it prints a read-only status table (hashrate, error, temp, VR, fan, J/TH, uptime) for each IP. With --run "<args>" it runs bitaxe_hashrate_benchmark.py on each miner in turn and then shows the resulting state. It shells out to the benchmark, so the main tool stays a single file. Adds tests for the pure helpers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fleet.py`:
- Around line 90-97: Update load_ips to strip each file line before checking
whether it is empty or begins with "#", so whitespace-indented comments are
excluded rather than treated as IPs. Add a regression test covering an indented
comment line and verify it is absent from the returned, order-preserving
de-duplicated addresses.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9aba3050-ef67-417e-8cc9-8dd3de1e8ee0
📒 Files selected for processing (3)
README.mdfleet.pytests/test_fleet.py
…vior - Add an anonymized "Real-world results" section (before/after error rates on BM1370 Gamma hardware, and the degraded-cooling / steady-state-vs-window caveat). - Document stabilization-window temperature monitoring and the implausible- hashrate skip under Safety Features and Data Processing. - Note hashrate std-dev and fan speed in the reduction steps.
- DESIGN.md said a thermal cap "retreats the same way"; grid now stops on a thermal/power cutoff. Align it with the code and the README. - Reword the README grid rationale to be literally accurate (passing combos are already recorded; adding voltage there would only add heat). - Fix a stale results-filename timestamp comment (%Y-%m-%d_%H -> %H%M%S).
…ion, fleet upgrades Implements the review's tuning recommendations, validated on BM1370 Gamma hardware. Selection / thermal: - Thermal-margin gate: a winner must average below --max-temp by --thermal-margin (default 1.5C), and VR temp is now a first-class limit, so a setting that only passed a short window isn't applied and then found to overheat at steady state. - thermallySettled: flag combos still heating at window end (temp-slope check), preferred as a selection tie-break. - Cooling-limited detection: when the fan is pegged and the chip stays pinned near the cap, print a verdict and stop refine from walking a degraded unit to the floor. - --soak <min>: after applying the winner, watch it at steady state (no reboot) and warn if temp/error drift over. - --bracket: refine also tests one frequency step down for better J/TH. - hashrateStd used as a selection tie-break. Robustness (review notes): - apply_settings short-circuits on a failed PATCH instead of polling a device that never took the settings. - monitored_stabilization requires two consecutive over-limit polls before aborting, so a transient spike doesn't falsely cap a combo. fleet.py: - Non-zero exit when any miner's run fails; status rows flag !err / !temp / !cooling (fan pegged + hot only, so a cool always-100% fan isn't flagged); missing values show n/a not 0. - Per-miner args in the --file list; before/after delta after --run; --log to append status snapshots for long-term cooling-trend tracking. 76 unit tests, all passing.
…back warning, temp-slope persistence - refine now treats an error/hashrate-clean combo that only lacks thermal headroom as capped (drop frequency) instead of raising voltage, which would only add heat - Ctrl+C during a soak now exits instead of being swallowed; the interrupt flag is sticky so the post-run soak is skipped - reset_to_best_setting warns with the specific limits missed (error / hashrate / thermal) when no combo passed, applying the least-bad result - tempSlope is now persisted per result in JSON and CSV and feeds the settled tie-break; efficiency is bucketed to 0.1 J/TH before tie-breaks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds ASIC error-rate awareness to the benchmark.
Right now the tool selects the highest-hashrate setting and reports J/TH separately, but it never looks at the hardware error rate. On BM1370 boards an aggressive undervolt can look good on hashrate and J/TH while the chip is actually throwing a double-digit hardware error rate, and that is the setting the current logic picks. This measures the error rate for each combination and selects the most efficient setting that stays under a configurable ceiling.
It stays a single file and a backward-compatible superset: existing commands behave the same, with the error rate now also reported. Still stdlib + requests only.
What it adds:
errorPercentageover the stable window, withstd/min/maxand the rawerrorCountdelta recorded. (On real firmwareerrorPercentageis a noisy rolling rate, so it is averaged over the window; details in docs/DESIGN.md.)--max-error, default 3.5%), falling back to the lowest-error setting when nothing clears the ceiling.refinemode: sweep voltage up to the first setting that clears the ceiling, probe down for a lower-voltage setting that still passes, and lower the frequency if the chip hits the temperature ceiling before the error clears.efficiencymode: hold the frequency and trim voltage down on an already-healthy miner.--check: read-only health snapshot (no changes, no reboot).--max-temp/--voltage-step/--frequency-step/--dry-runand CSV output.The decision logic is separated into pure functions with unit tests (
python -m pytest tests/, 38 passing), and it was validated on real BM1370 Gamma hardware.Happy to adjust the scope, split this into smaller PRs, or gate the new behavior behind a flag if you would prefer to keep the default unchanged.
Summary by CodeRabbit
New Features
Documentation
Tests