fix: ensure console logging is configurable #947
Conversation
|
Warning Rate limit exceeded@willkill07 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRehomes logging config under Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant WB as WorkflowBuilder
participant RL as RootLogger
participant SH as StreamHandlersStore
App->>WB: __aenter__()
WB->>RL: fetch root logger & handlers
WB->>RL: attach builder-managed handlers
alt telemetry console configured
WB->>SH: remove non-builder StreamHandlers\n(record in _removed_root_handlers)
else no telemetry console
WB->>SH: clamp existing StreamHandler levels\n(record previous levels)
end
WB->>RL: set root level >= min handler level
App->>WB: __aexit__()
WB->>RL: remove builder-managed handlers
WB->>SH: restore removed handlers and original levels
sequenceDiagram
autonumber
participant CFE as ConsoleFrontEnd
participant RL as RootLogger
participant STD as Stdout
CFE->>CFE: build result_message (prefix/line/suffix)
CFE->>RL: evaluate effective handler levels
alt INFO would be suppressed by root handlers
CFE->>STD: print(result_message)
else
CFE->>RL: logger.info(result_message)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Pre-merge checks and finishing touches and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
🧹 Nitpick comments (1)
src/nat/builder/workflow_builder.py (1)
196-198: Refactor: Console handler detection relies on fragile class name inspection.The current approach of checking if
"console"is in the class name is brittle and could break with custom handler implementations or naming variations.Consider one of these more robust approaches:
- Use a registration mechanism: Add a method or attribute to logging configs to explicitly declare if they're console handlers
- Check handler type more precisely: Examine the handler's stream attribute to see if it's
sys.stdoutorsys.stderr- Use configuration metadata: Add an explicit
is_console_handlerflag to logging configsExample for approach 2:
- has_console_handler = any( - hasattr(config, "__class__") and "console" in config.__class__.__name__.lower() - for config in telemetry_config.logging.values()) + # After handlers are created, check if any write to stdout/stderr + has_console_handler = any( + isinstance(handler, logging.StreamHandler) and + getattr(handler, 'stream', None) in (sys.stdout, sys.stderr) + for handler in self._logging_handlers.values())
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/front_ends/simple_auth/src/nat_simple_auth/configs/config.yml(1 hunks)src/nat/builder/workflow_builder.py(4 hunks)src/nat/data_models/config.py(1 hunks)src/nat/front_ends/console/console_front_end_plugin.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
src/nat/data_models/config.pyexamples/front_ends/simple_auth/src/nat_simple_auth/configs/config.ymlsrc/nat/builder/workflow_builder.pysrc/nat/front_ends/console/console_front_end_plugin.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/front_ends/console/console_front_end_plugin.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ (or packages//src/)
Files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/front_ends/console/console_front_end_plugin.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/front_ends/console/console_front_end_plugin.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/front_ends/console/console_front_end_plugin.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
src/nat/data_models/config.pysrc/nat/builder/workflow_builder.pysrc/nat/front_ends/console/console_front_end_plugin.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/data_models/config.pyexamples/front_ends/simple_auth/src/nat_simple_auth/configs/config.ymlsrc/nat/builder/workflow_builder.pysrc/nat/front_ends/console/console_front_end_plugin.py
**/*.{yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
In workflow/config YAML, set llms.._type: nat_test_llm to stub responses.
Files:
examples/front_ends/simple_auth/src/nat_simple_auth/configs/config.yml
**/configs/**
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Configuration files consumed by code must be stored next to that code in a configs/ folder
Files:
examples/front_ends/simple_auth/src/nat_simple_auth/configs/config.yml
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.
- If an example contains Python code, it should be placed in a subdirectory named
src/and should
contain apyproject.tomlfile. Optionally, it might also contain scripts in ascripts/directory.- If an example contains YAML files, they should be placed in a subdirectory named
configs/. - If an example contains sample data files, they should be placed in a subdirectory nameddata/, and should
be checked into git-lfs.
Files:
examples/front_ends/simple_auth/src/nat_simple_auth/configs/config.yml
⏰ 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). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (5)
src/nat/data_models/config.py (1)
190-190: LGTM! Stricter configuration validation.Adding
extra="forbid"ensures invalid configuration fields are caught early with clear validation errors, preventing silent misconfigurations.examples/front_ends/simple_auth/src/nat_simple_auth/configs/config.yml (1)
17-21: LGTM! Configuration restructuring aligns with new telemetry namespace.The logging configuration has been correctly moved under
general.telemetry.logging, maintaining the same settings while adhering to the new structure.src/nat/builder/workflow_builder.py (2)
230-237: Verify the handler level adjustment logic for non-console configurations.When no console handler is configured, the code adjusts existing StreamHandler levels to
min_handler_level(computed from all handlers including new ones). This behavior may not be correct in all scenarios.For example, if a DEBUG-level file handler is added but no console handler is configured, this would set existing console StreamHandlers to DEBUG level, potentially causing unexpected verbose console output.
Please verify:
- Is the intent to synchronize existing console handler levels with the new minimum level?
- Should this only apply if the new minimum level is higher (less verbose) than existing levels?
- What happens if users explicitly configured their root logger's StreamHandlers to specific levels?
Consider adding tests to verify behavior in these scenarios:
- Adding file handler at INFO, no console handler, existing console at DEBUG
- Adding file handler at WARNING, no console handler, existing console at INFO
- Adding multiple non-console handlers with different levels
249-259: LGTM! Proper cleanup and restoration of logging handlers.The exit logic correctly:
- Removes builder-managed handlers
- Restores previously removed/adjusted handlers
- Resets handler levels to their original values
The check
if handler not in root_logger.handlersprevents duplicate additions if a handler was somehow re-added.src/nat/front_ends/console/console_front_end_plugin.py (1)
98-110: Fallback print path never duplicates INFO logs
The conditional stdout print only executes when INFO-level logs are suppressed by handler.level > INFO, and otherwiselogger.infoemits output—both paths are mutually exclusive.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/nat/builder/workflow_builder.py (1)
216-220: Duplicate: Root logger level logic prevents handlers from receiving messages.This issue was already flagged in previous reviews. The use of
max()on line 220 incorrectly raises the root logger level instead of lowering it to allow messages through. Additionally, computingmin_handler_levelfrom all root logger handlers (line 217) instead of only newly added handlers can cause unintended behavior.Refer to the previous review comment for the detailed explanation and fix.
🧹 Nitpick comments (1)
src/nat/builder/workflow_builder.py (1)
230-237: Reconsider adjusting existing StreamHandler levels.When no console handler is configured, this code temporarily adjusts existing StreamHandler levels to
min_handler_level. While the original levels are restored in__aexit__, this changes console output verbosity during workflow execution in potentially unexpected ways.Consider this scenario:
- User starts with default console handler at WARNING level
- User configures only a file handler at INFO level
- Console output becomes more verbose (INFO level) during workflow execution
- After workflow completes, console returns to WARNING level
This temporary change in console verbosity might be confusing. The fallback
print()inconsole_front_end_plugin.py(lines 109-110) already ensures workflow results are visible when console handlers would suppress them.Consider simplifying the logic to:
- If a console handler is configured: remove existing console handlers (current behavior, lines 224-229)
- If no console handler is configured: leave existing console handlers unchanged and rely on the console frontend fallback for result visibility
This would make the behavior more predictable while still ensuring workflow results are always visible.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/nat/builder/workflow_builder.py(4 hunks)src/nat/front_ends/console/console_front_end_plugin.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
src/nat/front_ends/console/console_front_end_plugin.pysrc/nat/builder/workflow_builder.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
src/nat/front_ends/console/console_front_end_plugin.pysrc/nat/builder/workflow_builder.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ (or packages//src/)
Files:
src/nat/front_ends/console/console_front_end_plugin.pysrc/nat/builder/workflow_builder.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/front_ends/console/console_front_end_plugin.pysrc/nat/builder/workflow_builder.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/front_ends/console/console_front_end_plugin.pysrc/nat/builder/workflow_builder.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
src/nat/front_ends/console/console_front_end_plugin.pysrc/nat/builder/workflow_builder.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/front_ends/console/console_front_end_plugin.pysrc/nat/builder/workflow_builder.py
Signed-off-by: Will Killian <[email protected]>
Signed-off-by: Will Killian <[email protected]>
Signed-off-by: Will Killian <[email protected]>
willkill07
force-pushed
the
wkk_fix-console-logging
branch
from
45ebeb8 to
d96f82c
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/nat/builder/workflow_builder.py (1)
216-220: Critical: Incorrect operator prevents messages from reaching handlers.This is a duplicate of the past review comment. Using
max()at line 220 sets the root logger level to be MORE restrictive, blocking messages from reaching handlers configured at lower (more verbose) levels.For example:
- If a handler is at INFO (20) but root is at WARNING (30)
- Using
max(30, 20) = 30keeps root at WARNING- This blocks INFO messages from reaching the handler
Additionally, line 217 computes
min_handler_levelfrom ALL root logger handlers, including pre-existing handlers, which may not be the intended behavior.Apply this diff to fix the logic:
if self._logging_handlers: - min_handler_level = min((handler.level for handler in root_logger.handlers), default=logging.CRITICAL) + min_handler_level = min((handler.level for handler in self._logging_handlers.values()), default=logging.CRITICAL) # Ensure the root logger level allows messages through - root_logger.level = max(root_logger.level, min_handler_level) + root_logger.level = min(root_logger.level, min_handler_level)This ensures:
- Only newly added handlers are considered when computing the minimum level
- The root logger level is set to be at least as permissive as the most verbose handler
🧹 Nitpick comments (1)
src/nat/builder/workflow_builder.py (1)
191-198: Console detection heuristic is fragile.The console handler detection relies on inspecting class names for "console", which is fragile and may break if class naming conventions change. However, this is documented as a hack in the comment.
Consider adding a more robust detection mechanism, such as:
- Adding a
handler_typeattribute to logging configuration classes- Using a registry pattern for handler types
- Checking if the config class inherits from a specific base class
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/front_ends/simple_auth/src/nat_simple_auth/configs/config.yml(1 hunks)src/nat/builder/workflow_builder.py(4 hunks)src/nat/cli/entrypoint.py(1 hunks)src/nat/data_models/config.py(1 hunks)src/nat/front_ends/console/console_front_end_plugin.py(1 hunks)src/nat/observability/register.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/front_ends/simple_auth/src/nat_simple_auth/configs/config.yml
- src/nat/front_ends/console/console_front_end_plugin.py
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{py,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.{py,yaml,yml}: Configure response_seq as a list of strings; values cycle per call, and [] yields an empty string.
Configure delay_ms to inject per-call artificial latency in milliseconds for nat_test_llm.
Files:
src/nat/observability/register.pysrc/nat/builder/workflow_builder.pysrc/nat/cli/entrypoint.pysrc/nat/data_models/config.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/nat-test-llm.mdc)
**/*.py: Programmatic use: create TestLLMConfig(response_seq=[...], delay_ms=...), add with builder.add_llm("", cfg).
When retrieving the test LLM wrapper, use builder.get_llm(name, wrapper_type=LLMFrameworkEnum.) and call the framework’s method (e.g., ainvoke, achat, call).
**/*.py: In code comments/identifiers use NAT abbreviations as specified: nat for API namespace/CLI, nvidia-nat for package name, NAT for env var prefixes; do not use these abbreviations in documentation
Follow PEP 20 and PEP 8; run yapf with column_limit=120; use 4-space indentation; end files with a single trailing newline
Run ruff check --fix as linter (not formatter) using pyproject.toml config; fix warnings unless explicitly ignored
Respect naming: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
Treat pyright warnings as errors during development
Exception handling: use bare raise to re-raise; log with logger.error() when re-raising to avoid duplicate stack traces; use logger.exception() when catching without re-raising
Provide Google-style docstrings for every public module, class, function, and CLI command; first line concise and ending with a period; surround code entities with backticks
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work; profile CPU-heavy paths with cProfile or mprof before optimizing; cache expensive computations with functools.lru_cache or external cache; leverage NumPy vectorized operations when beneficial
Files:
src/nat/observability/register.pysrc/nat/builder/workflow_builder.pysrc/nat/cli/entrypoint.pysrc/nat/data_models/config.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All importable Python code must live under src/ (or packages//src/)
Files:
src/nat/observability/register.pysrc/nat/builder/workflow_builder.pysrc/nat/cli/entrypoint.pysrc/nat/data_models/config.py
src/nat/**/*
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Changes in src/nat should prioritize backward compatibility
Files:
src/nat/observability/register.pysrc/nat/builder/workflow_builder.pysrc/nat/cli/entrypoint.pysrc/nat/data_models/config.py
⚙️ CodeRabbit configuration file
This directory contains the core functionality of the toolkit. Changes should prioritize backward compatibility.
Files:
src/nat/observability/register.pysrc/nat/builder/workflow_builder.pysrc/nat/cli/entrypoint.pysrc/nat/data_models/config.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
All public APIs must have Python 3.11+ type hints on parameters and return values; prefer typing/collections.abc abstractions; use typing.Annotated when useful
Files:
src/nat/observability/register.pysrc/nat/builder/workflow_builder.pysrc/nat/cli/entrypoint.pysrc/nat/data_models/config.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
src/nat/observability/register.pysrc/nat/builder/workflow_builder.pysrc/nat/cli/entrypoint.pysrc/nat/data_models/config.py
🔇 Additional comments (7)
src/nat/data_models/config.py (1)
190-190: LGTM! Validation tightening aligns with PR objectives.Adding
extra="forbid"ensures that invalid configuration fields are rejected with clear validation errors, consistent with the existingConfigclass configuration at line 245.src/nat/cli/entrypoint.py (1)
55-59: LGTM! Explicit logging format improves consistency.The multi-argument
basicConfigcall explicitly defines the log format and date format, ensuring consistent logging presentation across the CLI. This aligns with the formatter changes insrc/nat/observability/register.py.src/nat/observability/register.py (2)
80-87: LGTM! Consistent formatter application.The formatter ensures that console logs match the standardized format defined in the CLI entrypoint, providing a consistent logging experience across different output handlers.
106-113: LGTM! Consistent formatter application.The formatter ensures that file logs match the standardized format defined in the CLI entrypoint, maintaining consistency across console and file logging outputs.
src/nat/builder/workflow_builder.py (3)
159-159: LGTM! Tracking removed handlers for restoration.Initializing
_removed_root_handlersas a list of tuples enables proper restoration of handler states during cleanup.
249-260: LGTM! Proper cleanup of logging handlers.The cleanup logic correctly:
- Removes builder-managed handlers from the root logger
- Restores previously removed handlers with their original levels
- Ensures the logging state is restored to its pre-entry condition
222-237: Root logger handler management verified
Search foraddHandlercalls found only the WorkflowBuilder attaching and restoring StreamHandlers, so removing or adjusting untracked handlers won’t impact other components.
willkill07
force-pushed
the
wkk_fix-console-logging
branch
from
ce27cdc to
98275a6
Compare
Signed-off-by: Will Killian <[email protected]>
willkill07
force-pushed
the
wkk_fix-console-logging
branch
from
98275a6 to
43b130e
Compare
|
/merge |
…r agent example Signed-off-by: David Gardner <[email protected]>
… profiler agent example" This reverts commit 2ecef88. Signed-off-by: David Gardner <[email protected]>
2 participants
Description
User specification of console logging did nothing. The root issue was we had multiple logger handlers. This PR addresses this bug and fixes it via the following:
extra="forbid"toGeneralConfig- Invalid configuration fields are now rejected with clear validation errorsDefault level:
Error level (when set in workflow config):
Error level (when set as envvar):
By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores