fix: CP2KDATA logging pollution #313
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughmain() now configures the root logger explicitly: sets level INFO, creates a custom formatter "%(asctime)s %(name)s - %(levelname)-5s : %(message)s", updates existing handlers’ formatter and level if present, or calls basicConfig with the same settings. No other behavior changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 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). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
dpgen2/entrypoint/main.py (2)
327-344: Don’t override existing logging configs; set levels only when you own the handlers.Unconditionally setting the root logger level and all handler levels to INFO can silently change user/dflow configurations. Limit mutations to StreamHandlers and only when no handlers exist; also allow env override for log level.
@@ - root_logger = logging.getLogger() - root_logger.setLevel(logging.INFO) + root_logger = logging.getLogger() formatter = logging.Formatter( "[%(asctime)s] %(name)s - %(levelname)-5s : %(message)s" ) - if root_logger.hasHandlers(): - # If handlers already exist (e.g., created by other modules), update their formatter and level - for handler in root_logger.handlers: - handler.setFormatter(formatter) - handler.setLevel(logging.INFO) + if root_logger.handlers: + # Respect previously configured handlers; only touch StreamHandlers and avoid changing explicit levels + for handler in list(root_logger.handlers): + if isinstance(handler, logging.StreamHandler): + handler.setFormatter(formatter) + if handler.level == logging.NOTSET: + handler.setLevel(logging.INFO) else: - # If no handlers exist, initialize logging with the desired format and level - logging.basicConfig( - level=logging.INFO, - format="[%(asctime)s] %(name)s - %(levelname)-5s : %(message)s", - ) + # If no handlers exist, we own logging config + level_name = os.getenv("DPGEN2_LOGLEVEL", "INFO").upper() + level = getattr(logging, level_name, logging.INFO) + root_logger.setLevel(level) + stream = logging.StreamHandler() + stream.setLevel(level) + stream.setFormatter(formatter) + root_logger.addHandler(stream)
334-344: If the goal is distinct styles for DPGEN2 vs third-party (e.g., cp2kdata), split handlers with filters.Updating one root handler’s formatter doesn’t actually separate styles. Optionally add two StreamHandlers: one for dpgen2 (INFO, detailed format) and one for others (WARNING, simpler format). This also curbs cp2kdata noise by default.
@@ - else: - # If no handlers exist, initialize logging with the desired format and level - logging.basicConfig( - level=logging.INFO, - format="[%(asctime)s] %(name)s - %(levelname)-5s : %(message)s", - ) + else: + level_name = os.getenv("DPGEN2_LOGLEVEL", "INFO").upper() + level = getattr(logging, level_name, logging.INFO) + root_logger.setLevel(level) + + # DPGEN2 handler (detailed) + dp_handler = logging.StreamHandler() + dp_handler.addFilter(logging.Filter("dpgen2")) # passes names starting with "dpgen2" + dp_handler.setLevel(level) + dp_handler.setFormatter(formatter) + root_logger.addHandler(dp_handler) + + # Third-party handler (quieter, simpler) + class _ExcludeDPGEN2(logging.Filter): + def filter(self, record): + return not record.name.startswith("dpgen2") + ext_handler = logging.StreamHandler() + ext_handler.addFilter(_ExcludeDPGEN2()) + ext_level_name = os.getenv("DPGEN2_3P_LOGLEVEL", "WARNING").upper() + ext_level = getattr(logging, ext_level_name, logging.WARNING) + ext_handler.setLevel(ext_level) + ext_handler.setFormatter(logging.Formatter("%(levelname)s: %(name)s: %(message)s")) + root_logger.addHandler(ext_handler)Minimal alternative: explicitly quiet cp2kdata without split handlers:
- logging.getLogger("cp2kdata").setLevel(getattr(logging, os.getenv("DPGEN2_CP2KDATA_LOGLEVEL", "WARNING").upper(), logging.WARNING))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
dpgen2/entrypoint/main.py(1 hunks)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #313 +/- ##
==========================================
- Coverage 84.23% 84.14% -0.10%
==========================================
Files 104 104
Lines 6129 6136 +7
==========================================
Hits 5163 5163
- Misses 966 973 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The cp2kdata module modifies the format of the global logger, which is an improper practice. DPGEN2 directly uses this global logger, and repeatedly modifying its format is not advisable. Besides, modifying the logging format in the |
Thanks, in that case, I'll take this pr for my personal usage. |
the cp2kdata python package will pollution the logging information:
This pull corrects the root logging info, and change the style of root logging to separate from sub logging.
Summary by CodeRabbit