Repository navigation
CLI: write the run log file synchronously - #135
Conversation
… enqueue queue enqueue=True pickles every line onto a multiprocessing queue and hands it to a worker thread. On a 200k-line microbenchmark that is ~46 us/line vs ~14 us/line for a direct write, and per-line cost is what bounds how fast the CLI drains a large run's log records.
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: writing the CLI run log file synchronously. Description check The description directly explains the logging change, its rationale, measured performance impact, risk, testing, and linked issues. Linked Issues check Direct issue [ #1185] requires synchronous writes for the CLI run log and unchanged log content and level.configure_logger_for_run()adds the file sink withenqueue=Falsewhile retaining the confi…Out of Scope Changes check The reported changes are limited to the run-log sink configuration, explanatory comments, and a unit test for the linked performance objective. The asynchronous streaming sink is not changed in behavi… Full details: Linked Issues check
-
Explanation
Direct issue [
#1185] requires synchronous writes for the CLI run log and unchanged log content and level.configure_logger_for_run()adds the file sink withenqueue=Falsewhile retaining the configured format and existing file path. The streaming sink remains asynchronous.test_log_file_sink_is_not_enqueuedverifies the required flag.
Full details: Out of Scope Changes check
-
Explanation
The reported changes are limited to the run-log sink configuration, explanatory comments, and a unit test for the linked performance objective. The asynchronous streaming sink is not changed in behavior. No unrelated change is demonstrated.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
greens
left a comment
greens
left a comment
There was a problem hiding this comment.
This is essentially mirroring in the CLI what you did in project-chip/certification-tool-backend#403, right?
Definitely a top priority for me for the next release to iron out our logging story.
Yes @greens |
What
configure_logger_for_run()now adds the run log file sink withenqueue=False(wasTrue), plus a unit test that checks the flag. Nothing else changes: same format, same level, same file.Why
With
enqueue=Trueevery log line is pickled onto a queue and handed to a worker thread. On a 712k-line run this limits how fast the CLI drains the log records, and the CLI finished about 30 s after the backend.Evidence
Raspberry Pi 5 / 8 GB, backend
41f91e7(v2.16-develop), TC-ACE-2.1 with real-time logs ON, one run per row:time realAbout 38 s (8%) less, in line with the lag it removes. Batch mode on the same test took 449.9 s with this PR. Short runs (TC-ACE-1.3, ~53k lines) show no difference (44.7 s against 45.5 s, within the ~7 s noise), as expected.
Risk
A slow disk would block the log write instead of filling a queue; the file is local to the machine running the CLI. The large-run gain is from a single run per row.
Tests
Added
test_log_file_sink_is_not_enqueued. Not run locally; CI runs the suite.Closes #1185
Part of #1119