Use RichHandler and remove existing log handler when init log - #693
Conversation
WalkthroughThe change updates the Changes
Estimated code review effort1 (~2 minutes) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📥 CommitsReviewing files that changed from the base of the PR and between 74890c937d330d163f0d91a742df1e84ce3c3a85 and d043457. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mainred
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
holmes/utils/console/logging.py (1)
48-61:force=Truebelongs tologging.basicConfig, notRichHandler
RichHandlerdoes not accept aforcekw-arg (seerich.logging.RichHandlersignature).
Placing it here raises aTypeErrorat runtime and is already flagged by mypy.
Theforceflag should be passed tologging.basicConfig, which is exactly what the PR intends—removing pre-existing handlers.Proposed patch (apply to all three
basicConfigblocks):- logging.basicConfig( - level=logging.DEBUG, # or INFO in the other blocks - format="%(message)s", - handlers=[ - RichHandler( - force=True, # ❌ wrong place - show_level=False, + logging.basicConfig( + level=logging.DEBUG, # or INFO in the other blocks + format="%(message)s", + force=True, # ✅ correct place + handlers=[ + RichHandler( + show_level=False,Repeat the same change for the VERBOSE and NORMAL branches (lines 63-76 and 80-93).
This will satisfy mypy and prevent runtime crashes while still wiping existing handlers.
Python ≥ 3.8 is required forforce=True; ensure CI/runtime image meets that.Also applies to: 63-76, 80-93
🧹 Nitpick comments (1)
holmes/utils/console/logging.py (1)
44-96: Consider DRYing the logging configurationThe three branches differ only in
leveland the post-config tweaks. Duplicating the wholebasicConfigblock invites drift.Example refactor:
def _rich_basic(level: int) -> None: logging.basicConfig( level=level, format="%(message)s", force=True, handlers=[RichHandler( show_level=False, markup=True, show_time=False, show_path=False, console=Console(width=None), )], ) # usage if verbosity == Verbosity.VERY_VERBOSE: _rich_basic(logging.DEBUG) elif verbosity == Verbosity.VERBOSE: _rich_basic(logging.INFO) logging.getLogger().setLevel(logging.DEBUG) suppress_noisy_logs() else: _rich_basic(logging.INFO) suppress_noisy_logs()Reduces duplication and centralises handler options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between be51687 and 74890c937d330d163f0d91a742df1e84ce3c3a85.
📒 Files selected for processing (1)
holmes/utils/console/logging.py(3 hunks)
🪛 GitHub Actions: Build and test HolmesGPT
holmes/utils/console/logging.py
[error] 52-52: mypy: Unexpected keyword argument "force" for "RichHandler" (call-arg)
[error] 67-67: mypy: Unexpected keyword argument "force" for "RichHandler" (call-arg)
[error] 84-84: mypy: Unexpected keyword argument "force" for "RichHandler" (call-arg)
🧰 Additional context used
🪛 GitHub Actions: Build and test HolmesGPT
holmes/utils/console/logging.py
[error] 52-52: mypy: Unexpected keyword argument "force" for "RichHandler" (call-arg)
[error] 67-67: mypy: Unexpected keyword argument "force" for "RichHandler" (call-arg)
[error] 84-84: mypy: Unexpected keyword argument "force" for "RichHandler" (call-arg)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
74890c9 to
d043457
Compare
|
@aantn this should has no impact on holmes which enforces RichHandler be the only log handler. |
2 participants
When importing holmes and leverage RichHandler to handle log, we need to remove existing log handlers to not let existing log handler break the outpout format of holmes CLI.
As a result, the log may not be printed out as expected, for example,
The force arg is explained here:
https://github.com/python/cpython/blob/e41c1ce585827f92dab9b7a7fc3df2bda2f817fe/Lib/logging/__init__.py#L2038-L2041
After the change: