Fix: log-system follow-ups for #703#706
Merged
ChaoWao merged 1 commit intohw-native-sys:mainfrom Apr 30, 2026
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the logging infrastructure to support custom verbosity levels (V0-V9 and NUL) by registering them as attributes on the logging module for pytest compatibility. It also modifies worker initialization to snapshot and pass logging levels explicitly to forked child processes. Documentation was updated to describe these changes, and feedback was provided to clarify the technical requirements for pytest compatibility within the docs.
Three issues reported after hw-native-sys#703 merged: 1. `pytest --log-level v0` was rejected ("'V0' is not recognized as a logging level name"). pytest's CLI validator uses `int(getattr(logging, name.upper(), name))`, so the level names must be exposed as **attributes on the logging module** — `logging.addLevelName` alone is not enough. Set both: keep `addLevelName` for nice `%(levelname)s` formatting, and `setattr(logging, ...)` so pytest's getattr lookup resolves. Mirror the registration in conftest.py top-level too — pytest validates --log-level before conftest's first `import simpler`. Also expose `logging.NULL` for `pytest --log-level null` (pytest upcases the value before lookup). 2. L3/L4 chip subprocesses ignored the user's simpler logger level. The parent's `Worker.init()` snapshot was only forwarded along the L2 path; `_chip_process_loop` and `_chip_process_loop_with_bootstrap` called `ChipWorker.init()` with the binding defaults (V5/INFO). Added `log_level` / `log_info_v` parameters to both child loops and snapshot the parent's logger config once before the fork loop in `_start_hierarchical`, then pass it explicitly into each forked child. 3. `docs/testing.md` Configuration Sources paragraph still claimed `Worker.run()` snapshots into `CallConfig.log_level` / `log_info_v` per-run. Those CallConfig fields were dropped in C4; the actual snapshot is one-shot at `Worker.init()`. Rewrote the paragraph to match, documented the L3/L4 fork plumbing, and updated the `--log-level` row in the Option Reference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2a1976d to
833369d
Compare
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
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.
Summary
Three follow-up issues found post-merge of #703 (the V0..V9 log redesign).
1.
pytest --log-level v0was brokenpytest's CLI validator does:
so accepted level names must be module attributes on
logging—logging.addLevelNamealone is insufficient.addLevelNameonly registers_levelToName/_nameToLeveldicts, which pytest doesn't consult here.Fix: also
setattr(logging, "V0", 15) ... setattr(logging, "V9", 24)plussetattr(logging, "NUL", 60)andsetattr(logging, "NULL", 60)(pytest upcasesnull→NULLbefore lookup). Done in two places:python/simpler/_log.py— for the standalonepython test_*.py --log-level v3path that goes through argparse afterimport simpler.conftest.pytop-level — pytest validates the CLI value before conftest's firstimport simpler, so the module-level registration there is the one that actually saves pytest. The two registrations are redundant by design.The
addLevelNamecalls are kept for the side-benefit:%(levelname)sformatters now printV3instead ofLevel 18.2. L3/L4 chip subprocesses ignored the simpler logger
Worker.init()correctly snapshotted Python'ssimplerlogger and forwarded it throughChipWorker.init(..., log_level, log_info_v)for the L2 path. The L3/L4 fork loops (_chip_process_loopand_chip_process_loop_with_bootstrap) calledChipWorker.init(...)without the new args, so each subprocess started itsHostLogger/ AICPU at the binding default (V5/INFO) regardless of what the user had configured.Fix: added
log_level/log_info_vparameters to both child loops (defaulted to1, 5for safety), and snapshot the parent's logger config once before the fork loop in_start_hierarchical— explicitly forwarded to each child invocation. After fork the child can't read the parent's logger state, so values must travel through the call site.3.
docs/testing.mdwas staleThe "Configuration sources" paragraph still described the C2-era model where
Worker.run()snapshots intoCallConfig.log_levelper-task. Those CallConfig fields were dropped in C4; the snapshot is now one-shot atWorker.init(). Rewrote the paragraph and the Option Reference row to match the actual implementation, and documented the L3/L4 fork plumbing.Test plan
pytest --log-level v0/v3/v9/info/warn/error/null/debugall accepted (8 variants × 11 cases pass)cpput: 21/21