feat: add flag for dumping dynamo engine config and environment #3286
Conversation
WalkthroughAdds a new “config dump” capability across Dynamo components. Introduces a common package with utilities to collect system/runtime/env information, optional package versions, and encode configurations to canonical JSON. Wires a new --dump-config-to CLI option and runtime calls in frontend, SGLang, TRTLLM, and vLLM to emit configuration snapshots to file or stdout. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as CLI Parser
participant Comp as Component (Frontend/SGLang/TRTLLM/vLLM)
participant CD as config_dump.dump_config
participant GCD as get_config_dump
participant SI as system_info/env/gpu/pkg
participant ENC as canonical_json_encoder
participant FS as Filesystem
User->>CLI: Launch process with args\n(--dump-config-to=path | unset)
CLI->>Comp: Parsed flags/config
Note over CLI,Comp: add_config_dump_args() added the flag
Comp->>CD: dump_config(flags.dump_config_to, config)
CD->>GCD: Build payload from config
GCD->>SI: get_system_info(), get_runtime_info(),\nget_environment_vars(), get_gpu_info(), get_package_info()
SI-->>GCD: Collected info (partial on errors)
GCD->>ENC: Encode via singledispatch preprocessing
ENC-->>GCD: Canonical JSON string
alt dump_config_to provided
CD->>FS: Write JSON to path (mkdir -p as needed)
FS-->>CD: OK / raise error
opt on write error
CD->>User: Emit CONFIG_DUMP to stdout
end
else no target path
CD->>User: Emit CONFIG_DUMP to stdout
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 3
🧹 Nitpick comments (6)
components/common/src/dynamo/common/config_dump/environment.py (1)
11-11: Address the TODO comment.The TODO suggests uncertainty about which environment variable prefixes should be captured. Consider adding commonly relevant prefixes like
PYTHON_,PATH,LD_LIBRARY_PATH,TORCH_, or backend-specific ones.Would you like me to suggest a more comprehensive list of prefixes based on the backends used in this project (TensorRT-LLM, vLLM, SGLang)?
components/backends/sglang/src/dynamo/sglang/args.py (1)
105-116: Simplify the serving_mode encoding.The encoder converts
serving_modeto eitherconfig.serving_mode.valueor the string"None". Consider usingconfig.serving_mode.value if config.serving_mode else Noneto avoid the literal string"None", which could be confusing when deserializing.Apply this diff:
-@register_encoder(Config) -def _preprocess_for_encode_config( - config: Config, -) -> Dict[str, Any]: # pyright: ignore[reportUnusedFunction] - return { - "server_args": config.server_args, - "dynamo_args": config.dynamo_args, - "serving_mode": config.serving_mode.value - if config.serving_mode is not None - else "None", - } +@register_encoder(Config) +def _preprocess_for_encode_config( + config: Config, +) -> Dict[str, Any]: # pyright: ignore[reportUnusedFunction] + return { + "server_args": config.server_args, + "dynamo_args": config.dynamo_args, + "serving_mode": config.serving_mode.value if config.serving_mode else None, + }components/common/src/dynamo/common/config_dump/__init__.py (1)
10-15: Consider sorting__all__alphabetically.Alphabetically sorting
__all__improves maintainability and reduces potential merge conflicts.Apply this diff to sort the list:
__all__ = [ + "add_config_dump_args", "dump_config", "get_config_dump", "register_encoder", - "add_config_dump_args", ]components/common/src/dynamo/common/config_dump/config_dumper.py (3)
69-71: Uselogging.exceptionfor better error diagnostics.When catching exceptions,
logging.exceptionautomatically includes the traceback, which is valuable for debugging file I/O failures.Apply this diff:
except Exception as e: - logger.error(f"Failed to dump debug config to {dump_config_to}: {e}") + logger.exception(f"Failed to dump debug config to {dump_config_to}: {e}") logger.info("DEBUG_CONFIG:" + debug_config_payload)
82-82: Fix docstring parameter name.The docstring references
extra_versionsbut the parameter is namedextra_info.Apply this diff:
Args: config: Any json-serializable object containing the backend configuration - extra_versions: Optional dict of additional version information to include + extra_info: Optional dict of additional version information to include Returns:
110-111: Uselogging.exceptionfor better error diagnostics.Similar to the other exception handler, using
logging.exceptionwill automatically include the traceback.Apply this diff:
except Exception as e: - logger.error(f"Error collecting debug info: {e}") + logger.exception(f"Error collecting debug info: {e}") # Return a basic error response with at least system info
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.devcontainer/post-create.sh(1 hunks).gitignore(1 hunks)README.md(1 hunks)components/backends/sglang/src/dynamo/sglang/args.py(5 hunks)components/backends/sglang/src/dynamo/sglang/main.py(2 hunks)components/backends/trtllm/src/dynamo/trtllm/main.py(2 hunks)components/backends/trtllm/src/dynamo/trtllm/utils/trtllm_utils.py(5 hunks)components/backends/vllm/src/dynamo/vllm/args.py(4 hunks)components/backends/vllm/src/dynamo/vllm/main.py(2 hunks)components/common/src/dynamo/common/__init__.py(1 hunks)components/common/src/dynamo/common/config_dump/__init__.py(1 hunks)components/common/src/dynamo/common/config_dump/config_dumper.py(1 hunks)components/common/src/dynamo/common/config_dump/environment.py(1 hunks)components/common/src/dynamo/common/config_dump/system_info.py(1 hunks)components/frontend/src/dynamo/frontend/main.py(3 hunks)container/Dockerfile.sglang(1 hunks)container/Dockerfile.trtllm(1 hunks)container/Dockerfile.vllm(1 hunks)dynamo.code-workspace(1 hunks)hatch_build.py(1 hunks)pyproject.toml(1 hunks)pyrightconfig.json(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-05T22:51:59.230Z
Learnt from: dmitry-tokarev-nv
PR: ai-dynamo/dynamo#2300
File: pyproject.toml:64-66
Timestamp: 2025-08-05T22:51:59.230Z
Learning: The ai-dynamo/dynamo project does not ship ARM64 wheels, so platform markers to restrict dependencies to x86_64 are not needed in pyproject.toml dependencies.
Applied to files:
pyproject.toml
📚 Learning: 2025-08-18T16:52:15.659Z
Learnt from: nnshah1
PR: ai-dynamo/dynamo#2489
File: container/deps/vllm/install_vllm.sh:151-152
Timestamp: 2025-08-18T16:52:15.659Z
Learning: The VLLM_PRECOMPILED_WHEEL_LOCATION environment variable, when exported, automatically triggers vLLM's build system to use the precompiled wheel instead of building from source, even when using standard `uv pip install .` commands in container/deps/vllm/install_vllm.sh.
Applied to files:
README.md
🧬 Code graph analysis (9)
components/backends/trtllm/src/dynamo/trtllm/main.py (1)
components/common/src/dynamo/common/config_dump/config_dumper.py (1)
dump_config(51-73)
components/frontend/src/dynamo/frontend/main.py (1)
components/common/src/dynamo/common/config_dump/config_dumper.py (2)
dump_config(51-73)add_config_dump_args(120-132)
components/common/src/dynamo/common/config_dump/__init__.py (1)
components/common/src/dynamo/common/config_dump/config_dumper.py (4)
add_config_dump_args(120-132)dump_config(51-73)get_config_dump(76-117)register_encoder(151-161)
components/backends/trtllm/src/dynamo/trtllm/utils/trtllm_utils.py (1)
components/common/src/dynamo/common/config_dump/config_dumper.py (1)
add_config_dump_args(120-132)
components/backends/sglang/src/dynamo/sglang/args.py (3)
components/common/src/dynamo/common/config_dump/config_dumper.py (1)
register_encoder(151-161)components/backends/vllm/src/dynamo/vllm/args.py (2)
Config(41-69)_preprocess_for_encode_config(73-74)components/backends/trtllm/src/dynamo/trtllm/utils/trtllm_utils.py (1)
Config(29-96)
components/backends/sglang/src/dynamo/sglang/main.py (1)
components/common/src/dynamo/common/config_dump/config_dumper.py (1)
dump_config(51-73)
components/backends/vllm/src/dynamo/vllm/main.py (1)
components/common/src/dynamo/common/config_dump/config_dumper.py (1)
dump_config(51-73)
components/backends/vllm/src/dynamo/vllm/args.py (1)
components/common/src/dynamo/common/config_dump/config_dumper.py (2)
register_encoder(151-161)add_config_dump_args(120-132)
components/common/src/dynamo/common/config_dump/config_dumper.py (2)
components/common/src/dynamo/common/config_dump/environment.py (1)
get_environment_vars(8-27)components/common/src/dynamo/common/config_dump/system_info.py (2)
get_runtime_info(19-25)get_system_info(9-16)
🪛 Ruff (0.13.1)
components/common/src/dynamo/common/config_dump/__init__.py
10-15: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
components/common/src/dynamo/common/config_dump/config_dumper.py
26-26: Consider moving this statement to an else block
(TRY300)
36-36: Consider moving this statement to an else block
(TRY300)
46-46: Consider moving this statement to an else block
(TRY300)
69-69: Do not catch blind exception: Exception
(BLE001)
70-70: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
76-76: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
110-110: Do not catch blind exception: Exception
(BLE001)
111-111: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
114-114: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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: Build and Test - dynamo
🔇 Additional comments (37)
.devcontainer/post-create.sh (1)
114-114: LGTM! Consistent PYTHONPATH update.The addition of the common components path to the backup PYTHONPATH aligns with the PR's introduction of a new shared common package and matches similar updates in Dockerfiles and README.md.
.gitignore (1)
102-105: LGTM! Standard ignore entries.The additions appropriately ignore benchmark results and Direnv configuration files, which are typically local artifacts that should not be committed.
components/common/src/dynamo/common/__init__.py (1)
1-13: LGTM! Standard package initialization.The module follows Python package conventions with proper docstring, license headers, and controlled public exports via
__all__.pyproject.toml (1)
80-80: LGTM! Correct packaging of new common module.The addition of the common components package to the wheel build targets ensures the new shared utilities are properly distributed. The placement at the beginning of the list is logical.
components/backends/vllm/src/dynamo/vllm/main.py (2)
15-15: LGTM!The import of
dump_configfrom the new common config-dump module is correct and follows the expected pattern.
82-82: Verification complete:dump_config_toattribute is properly defined.The
Configclass includes thedump_config_toattribute at line 69 asdump_config_to: Optional[str] = None, and it is correctly assigned from parsed arguments at line 206. No issues found.components/backends/sglang/src/dynamo/sglang/main.py (2)
14-14: LGTM!The import of
dump_configis correct and consistent with the vLLM backend implementation.
43-43: The attribute pathconfig.dynamo_args.dump_config_tois correct.Verification confirms that the
Configclass storesdynamo_argsas an instance attribute (line 93 in args.py), anddump_config_tois defined as an attribute of theDynamoArgsclass (line 81). The nested attribute path matches the SGLang backend's structure and is properly implemented.README.md (1)
304-304: LGTM!The documentation correctly reflects the updated
PYTHONPATHrequirements, including the newcomponents/common/srcpath needed for the config-dump functionality. This is consistent with the changes in Dockerfiles and workspace configuration.container/Dockerfile.sglang (1)
324-324: PYTHONPATH ordering is correct.The addition of
components/common/srcat the beginning ofPYTHONPATHis necessary and appropriate. Multiple components (frontend, vllm, sglang, trtllm) import fromdynamo.common, so this path must be included.The original concern about
components/metrics/srcordering is not applicable—that directory contains only Rust source files (lib.rs, main.rs), not Python code, so its position inPYTHONPATHdoes not affect Python import resolution.Likely an incorrect or invalid review comment.
components/frontend/src/dynamo/frontend/main.py (3)
33-34: LGTM!Clean import of the config dump functionality from the common module.
205-205: LGTM!Proper wiring of config dump arguments into the argument parser.
219-219: LGTM!Config dumping is appropriately invoked at the start of async_main before engine initialization, ensuring the configuration snapshot is captured early in the startup sequence.
hatch_build.py (1)
10-10: LGTM!Properly adds the common component to the version generation list, ensuring the new shared module receives version metadata like other components.
components/backends/trtllm/src/dynamo/trtllm/main.py (2)
26-26: LGTM!Clean import of the config dump functionality.
274-276: LGTM!Config dumping is appropriately placed after NIXL connector initialization and before entering the engine context, capturing both the parsed engine arguments and dynamo configuration for debugging and benchmarking purposes.
pyrightconfig.json (1)
1-17: LGTM!Well-structured Pyright configuration that enables proper type checking across all component source directories, including the newly added common module and existing frontend, planner, and backend components.
container/Dockerfile.trtllm (1)
326-326: LGTM!Correctly prepends the common component source path to PYTHONPATH, enabling runtime discovery of the new dynamo.common module and config_dump utilities during development.
container/Dockerfile.vllm (1)
359-359: LGTM!The PYTHONPATH update correctly prepends the common component path, ensuring early module resolution for the newly introduced
dynamo.common.config_dumpfacility.components/common/src/dynamo/common/config_dump/system_info.py (2)
9-16: LGTM!The system information collection uses standard library calls appropriately. Note that
platform.processor()may return an empty string on some platforms, which is acceptable for debug output.
19-25: Verify that command-line arguments don't contain secrets.The function exposes
sys.argvin the runtime info, which may include sensitive data like passwords, tokens, or API keys passed as command-line arguments. Ensure that upstream callers sanitize or warn about this exposure, especially when dumping to files.Review how
get_runtime_info()is consumed in the config dump flow to confirm sensitive CLI args are handled appropriately.components/backends/vllm/src/dynamo/vllm/args.py (5)
7-7: LGTM!The imports correctly add support for type hints and config dump integration.
Also applies to: 14-14, 16-16
68-69: LGTM!The new
dump_config_tofield is appropriately typed and positioned within the Config class.
72-74: Consider filtering sensitive fields from the config dict.The encoder exposes the entire
Config.__dict__, which includesengine_argsthat may contain sensitive information. Verify that downstream serialization or the dump mechanism sanitizes secrets before writing to files or logs.Check whether the config dump facility in
dynamo.common.config_dumpperforms any sanitization of sensitive fields like API keys, tokens, or credentials that might be present inengine_argsor other config attributes.
135-135: LGTM!CLI argument wiring is correctly positioned and follows the pattern established in other backends.
206-206: LGTM!The assignment correctly propagates the CLI argument value into the Config object.
components/backends/trtllm/src/dynamo/trtllm/utils/trtllm_utils.py (5)
11-11: LGTM!The import correctly adds CLI argument support for config dumping.
63-63: LGTM!The new field is properly initialized with the correct type and default value.
94-95: LGTM!Including
dump_config_toin the string representation aids debugging and logging.
302-302: LGTM!CLI argument wiring is correctly positioned and consistent with other backends.
373-373: LGTM!The assignment correctly propagates the CLI argument value into the Config object.
components/backends/sglang/src/dynamo/sglang/args.py (4)
13-13: LGTM!The imports correctly add type hints and encoder registration support.
Also applies to: 18-18
59-64: LGTM!The CLI argument definition follows the established DYNAMO_ARGS pattern and is consistent with the shared config dump interface.
81-81: LGTM!The new field is appropriately typed and positioned within the DynamoArgs dataclass.
231-231: LGTM!The assignment correctly propagates the CLI argument into the DynamoArgs instance.
components/common/src/dynamo/common/config_dump/config_dumper.py (2)
120-133: LGTM!The argument definition is clear and correctly configured.
135-186: LGTM! Well-designed extensible encoding system.The
singledispatch-based encoder registration provides a clean, extensible mechanism for custom serialization. The fallback logic (dataclasses →__dict__→str()) is appropriate, and the canonical JSON encoder configuration ensures deterministic output (sorted keys, consistent formatting).
components/common/src/dynamo/common/config_dump/config_dumper.py
Outdated
Show resolved
Hide resolved
Aphoh
changed the title
Aphoh
force-pushed
the
warnold/create-debug-config-endpoint
branch
3 times, most recently
from
079ea24 to
1a1ba1b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/src/dynamo/common/config_dump/environment.py (1)
27-34: Consider expanding SENSITIVE_PATTERNS for more comprehensive coverage.While the current patterns cover common cases, consider adding:
"PRIVATE_KEY""ACCESS""BEARER""JWT"This would provide more comprehensive protection against accidental credential exposure.
Apply this diff to expand the patterns:
SENSITIVE_PATTERNS = [ "TOKEN", "API_KEY", "SECRET", "PASSWORD", "CREDENTIAL", "AUTH", + "PRIVATE_KEY", + "ACCESS", + "BEARER", + "JWT", ]components/src/dynamo/common/config_dump/config_dumper.py (2)
80-109: Simplify exception logging.The function correctly implements the dump-to-file-or-log behavior with appropriate parent directory creation. However, the exception object is redundantly included in the log message.
Apply this diff to remove the redundant exception information:
except (OSError, IOError) as e: - logger.exception(f"Failed to dump config to {dump_config_to}: {e}") + logger.exception(f"Failed to dump config to {dump_config_to}") logger.info(f"CONFIG_DUMP: {config_dump_payload}") except Exception as e: - logger.exception(f"Unexpected error dumping config: {e}") + logger.exception(f"Unexpected error dumping config") logger.info(f"CONFIG_DUMP: {config_dump_payload}")Note:
logging.exceptionautomatically includes the exception details in the log output.
112-160: Uselogging.exceptionfor better error diagnostics.The function correctly aggregates comprehensive configuration information with appropriate fallback behavior. However, when catching and logging an exception,
logging.exceptionshould be used instead oflogging.errorto include the stack trace.Apply this diff:
except Exception as e: - logger.error(f"Error collecting config dump: {e}") + logger.exception("Error collecting config dump") # Return a basic error response with at least system info error_info = { "error": f"Failed to collect config dump: {str(e)}",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore(1 hunks)components/src/dynamo/common/__init__.py(1 hunks)components/src/dynamo/common/config_dump/__init__.py(1 hunks)components/src/dynamo/common/config_dump/config_dumper.py(1 hunks)components/src/dynamo/common/config_dump/environment.py(1 hunks)components/src/dynamo/common/config_dump/system_info.py(1 hunks)components/src/dynamo/frontend/main.py(3 hunks)components/src/dynamo/sglang/args.py(5 hunks)components/src/dynamo/sglang/main.py(2 hunks)components/src/dynamo/trtllm/main.py(2 hunks)components/src/dynamo/trtllm/utils/trtllm_utils.py(5 hunks)components/src/dynamo/vllm/args.py(4 hunks)components/src/dynamo/vllm/main.py(2 hunks)hatch_build.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.13.3)
components/src/dynamo/common/config_dump/config_dumper.py
35-35: Consider moving this statement to an else block
(TRY300)
53-53: Consider moving this statement to an else block
(TRY300)
71-71: Consider moving this statement to an else block
(TRY300)
103-103: Redundant exception object included in logging.exception call
(TRY401)
106-106: Redundant exception object included in logging.exception call
(TRY401)
153-153: Do not catch blind exception: Exception
(BLE001)
154-154: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
157-157: Use explicit conversion flag
Replace with conversion flag
(RUF010)
components/src/dynamo/common/config_dump/system_info.py
28-28: Do not catch blind exception: Exception
(BLE001)
34-34: Do not catch blind exception: Exception
(BLE001)
40-40: Do not catch blind exception: Exception
(BLE001)
46-46: Do not catch blind exception: Exception
(BLE001)
54-54: Do not catch blind exception: Exception
(BLE001)
79-79: Do not catch blind exception: Exception
(BLE001)
85-85: Do not catch blind exception: Exception
(BLE001)
91-91: Do not catch blind exception: Exception
(BLE001)
113-117: Starting a process with a partial executable path
(S607)
137-137: Do not catch blind exception: Exception
(BLE001)
⏰ 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: Build and Test - dynamo
🔇 Additional comments (39)
hatch_build.py (1)
10-10: LGTM!The addition of "common" to the COMPONENTS list aligns with the introduction of the new common package.
components/src/dynamo/vllm/main.py (2)
15-15: LGTM!Import is correctly placed with other dynamo imports.
82-82: LGTM!The config dump is appropriately placed after signal handlers are set up and before worker initialization, ensuring the runtime configuration is captured early.
components/src/dynamo/frontend/main.py (3)
33-34: LGTM!Imports are correctly placed with other dynamo imports.
212-212: LGTM!The CLI argument integration is correctly placed with other argument definitions.
226-226: LGTM!Config dump is appropriately placed early in the async_main function, capturing the configuration immediately after parsing.
components/src/dynamo/sglang/main.py (2)
14-14: LGTM!Import is correctly placed with other dynamo imports.
50-50: LGTM!Config dump is appropriately placed after argument parsing and before initialization branches.
components/src/dynamo/trtllm/main.py (2)
26-26: LGTM!Import is correctly placed with other dynamo imports.
274-276: Confirm custom dump_config payload in trtllmtrtllm/main.py (lines 274–276) wraps
engine_argsandconfigin a dict, whereas other backends pass the config object directly; verify this deviation is intentional and thatdump_confighandles both fields correctly.components/src/dynamo/vllm/args.py (6)
7-7: LGTM!The typing imports are correctly added to support the encoder function signature.
16-16: LGTM!Import is correctly placed with other dynamo imports.
68-69: LGTM!The
dump_config_tofield is appropriately added to the Config class for storing the config dump destination.
72-74: LGTM!The encoder function correctly returns the config's dict representation for serialization. Exposing
__dict__is acceptable here since this is specifically for debugging/auditing purposes.
135-135: LGTM!The CLI argument integration is correctly placed with other argument definitions.
220-220: LGTM!The assignment correctly propagates the CLI argument value to the config instance.
components/src/dynamo/common/__init__.py (1)
1-17: LGTM!The package initialization correctly exposes the public API (
__version__andconfig_dumpmodule) with clear documentation.components/src/dynamo/common/config_dump/environment.py (2)
37-78: LGTM!The function implementation is correct and handles the different use cases appropriately. The default behavior of redacting sensitive values is a good security practice.
81-91: LGTM!The helper function correctly implements case-insensitive pattern matching for sensitive variable detection.
components/src/dynamo/trtllm/utils/trtllm_utils.py (5)
11-11: LGTM!The import is correctly placed and properly used later in the file at line 304.
63-63: LGTM!The new
dump_config_tofield is properly typed and initialized with a sensible default value.
96-96: LGTM!The
dump_config_tofield is correctly included in the string representation, maintaining consistency with the rest of the Config fields.
304-304: LGTM!The CLI argument registration is cleanly integrated using the common utility function, maintaining consistency with the broader config dump infrastructure.
381-381: LGTM!The argument propagation follows the established pattern and correctly assigns the parsed CLI argument to the config field.
components/src/dynamo/sglang/args.py (5)
18-18: LGTM!The import is correctly placed and used for registering the custom Config encoder.
83-88: LGTM!The CLI argument definition is well-structured and provides clear documentation for users. The help text accurately describes the behavior when the flag is not specified.
112-113: LGTM!The field is properly typed and well-documented with the section comment.
139-150: LGTM!The custom encoder correctly decomposes the SGLang Config into serializable components. The enum-to-string conversion for
serving_modeis appropriate, and thepyright: ignoredirective is correctly applied for this singledispatch registration pattern.
295-295: LGTM!The argument is correctly propagated to the DynamoArgs dataclass following the established pattern.
components/src/dynamo/common/config_dump/__init__.py (1)
1-33: LGTM!The package initialization file is well-structured with a clear module docstring, organized imports, and an explicit
__all__declaration that defines the public API surface. This follows Python best practices for package organization.components/src/dynamo/common/config_dump/system_info.py (4)
13-57: LGTM!The function correctly implements graceful degradation by catching exceptions for each information-gathering step and providing sensible defaults. The broad exception handling is appropriate for diagnostic code where gathering partial information is preferable to failing completely.
60-95: LGTM!The function follows the same robust pattern as
get_system_info, ensuring that runtime information is collected with appropriate fallbacks. The defensive error handling is well-suited for diagnostic purposes.
98-140: LGTM!The GPU information gathering is appropriately best-effort with proper timeout protection. The use of a partial executable path for
nvidia-smi(flagged by static analysis) is acceptable for diagnostic code that needs to work across different system configurations wherenvidia-smiis in the PATH.
143-155: LGTM!The function correctly enumerates installed packages using the standard
importlib.metadataAPI. The implementation is straightforward and appropriate for the diagnostic use case.components/src/dynamo/common/config_dump/config_dumper.py (5)
26-77: LGTM!The version getter functions are well-structured with appropriate error handling for optional dependencies. The static analysis suggestion (TRY300) to move return statements to else blocks is a minor style preference; the current pattern is clear and concise.
163-175: LGTM!The CLI argument is well-defined with clear documentation for users.
178-191: LGTM!The base preprocessing function provides sensible fallback behavior with dataclass support and a reasonable fallback chain for unknown types. The warning log helps identify types that may need explicit encoders.
194-218: LGTM!The encoder registration system is well-designed with appropriate handlers for common non-serializable types (sets and enums). The
logger.verbosecall appears to be a custom log level configured elsewhere in the Dynamo logging infrastructure, which is fine for this use case.
222-229: LGTM!The JSON encoder is properly configured for deterministic, compact output with custom type handling. The settings are appropriate for a configuration dump utility where consistency and readability are important.
Aphoh
force-pushed
the
warnold/create-debug-config-endpoint
branch
from
22a75fa to
2265ec5
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/src/dynamo/common/config_dump/system_info.py (1)
143-154: Consider tightening the return type annotation.The function always returns a dictionary (never
None), so the return type could beDict[str, str]instead ofOptional[Dict[str, Any]]for better type precision.Apply this diff:
-def get_package_info() -> Optional[Dict[str, Any]]: +def get_package_info() -> Dict[str, str]: """ Get package information. Returns: Dictionary containing installed packages and their versions. """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore(1 hunks)components/src/dynamo/common/__init__.py(1 hunks)components/src/dynamo/common/config_dump/__init__.py(1 hunks)components/src/dynamo/common/config_dump/config_dumper.py(1 hunks)components/src/dynamo/common/config_dump/environment.py(1 hunks)components/src/dynamo/common/config_dump/system_info.py(1 hunks)components/src/dynamo/frontend/main.py(3 hunks)components/src/dynamo/sglang/args.py(5 hunks)components/src/dynamo/sglang/main.py(2 hunks)components/src/dynamo/trtllm/main.py(2 hunks)components/src/dynamo/trtllm/utils/trtllm_utils.py(5 hunks)components/src/dynamo/vllm/args.py(4 hunks)components/src/dynamo/vllm/main.py(2 hunks)hatch_build.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- components/src/dynamo/common/config_dump/init.py
- components/src/dynamo/vllm/args.py
- components/src/dynamo/frontend/main.py
- components/src/dynamo/common/config_dump/environment.py
- hatch_build.py
- components/src/dynamo/vllm/main.py
- components/src/dynamo/sglang/args.py
- components/src/dynamo/trtllm/utils/trtllm_utils.py
- components/src/dynamo/common/init.py
- .gitignore
- components/src/dynamo/trtllm/main.py
🧰 Additional context used
🪛 Ruff (0.13.3)
components/src/dynamo/common/config_dump/system_info.py
28-28: Do not catch blind exception: Exception
(BLE001)
34-34: Do not catch blind exception: Exception
(BLE001)
40-40: Do not catch blind exception: Exception
(BLE001)
46-46: Do not catch blind exception: Exception
(BLE001)
54-54: Do not catch blind exception: Exception
(BLE001)
79-79: Do not catch blind exception: Exception
(BLE001)
85-85: Do not catch blind exception: Exception
(BLE001)
91-91: Do not catch blind exception: Exception
(BLE001)
113-117: Starting a process with a partial executable path
(S607)
137-137: Do not catch blind exception: Exception
(BLE001)
components/src/dynamo/common/config_dump/config_dumper.py
35-35: Consider moving this statement to an else block
(TRY300)
53-53: Consider moving this statement to an else block
(TRY300)
71-71: Consider moving this statement to an else block
(TRY300)
153-153: Do not catch blind exception: Exception
(BLE001)
154-154: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
157-157: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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: Build and Test - dynamo
🔇 Additional comments (10)
components/src/dynamo/sglang/main.py (2)
12-12: LGTM!The import is clean and correctly references the new shared config_dump module introduced in this PR.
49-49: LGTM!The placement of
dump_configis appropriate—after argument parsing but before engine initialization. This ensures the configuration snapshot is captured early in the startup sequence without blocking critical initialization paths. Error handling is managed withindump_configitself.components/src/dynamo/common/config_dump/system_info.py (3)
13-57: LGTM!The defensive error handling with fallback to "unknown" values is appropriate for a best-effort information gathering function. The broad exception handling flagged by static analysis (BLE001) is intentional here—each platform API call is isolated, logged, and provides sensible defaults to ensure the function always returns usable data.
60-95: LGTM!The function follows the same defensive pattern as
get_system_info, ensuring partial runtime information is always available even if individual attribute accesses fail. The broad exception handling is justified for this use case.
98-140: LGTM!The GPU information gathering is appropriately defensive:
- The 5-second timeout prevents hanging if nvidia-smi is unresponsive
- The subprocess call to
nvidia-smiis safe (standard system tool, not user-controlled input)—the S607 static analysis warning is a false positive- Parsing with
len(parts) >= 3safely handles the expected CSV format- The broad exception handling (BLE001) is justified for this optional, best-effort function
- Debug-level logging is appropriate since failures are expected on non-GPU systems
components/src/dynamo/common/config_dump/config_dumper.py (5)
26-77: LGTM!The version getter functions follow a consistent pattern for querying optional dependencies. The static analysis suggestion (TRY300) to move returns into
elseblocks is a style preference; the current approach is clear and maintainable.
80-109: LGTM!The error handling is appropriately defensive:
- Creates parent directories with
exist_ok=Trueto avoid path errors- Uses explicit UTF-8 encoding for cross-platform consistency
- Catches specific file I/O errors (OSError, IOError) first, then falls back to broad Exception handling
- Ensures configuration data reaches stdout even if file writes fail
This approach guarantees the config dump is always logged somewhere, which is critical for debugging.
163-175: LGTM!The CLI argument definition is clear and well-documented. The help text accurately describes the behavior (file write vs. stdout logging).
178-218: LGTM!The
functools.singledispatchapproach provides clean extensibility for custom type encoding. The built-in handlers are appropriate:
- Dataclass →
asdict()preserves structure- Set → sorted list ensures deterministic output
- Enum → string provides readable serialization
- Unknown types → warning + fallback to
__dict__orstr(obj)ensures graceful degradationThe warning for unknown types is intentional and useful for identifying types that need explicit encoders.
222-229: LGTM!The JSON encoder configuration is well-suited for canonical config dumps:
sort_keys=Trueensures deterministic output for diffing and version controlallow_nan=Falseprevents invalid JSON (strict compliance)- Compact separators reduce file size
ensure_ascii=Falseallows unicode characters (better for internationalized content)- Custom
defaulthandler via_preprocess_for_encodeprovides extensibilityThis produces reproducible, valid, compact JSON.
Aphoh
force-pushed
the
warnold/create-debug-config-endpoint
branch
from
8a47d8c to
a1324e5
Compare
Aphoh
force-pushed
the
warnold/create-debug-config-endpoint
branch
from
a1324e5 to
1d3fc27
Compare
|
/ok to test 1d3fc27 |
Aphoh
force-pushed
the
warnold/create-debug-config-endpoint
branch
from
1d3fc27 to
af5dcf5
Compare
Signed-off-by: William Arnold <[email protected]>
Signed-off-by: William Arnold <[email protected]>
Signed-off-by: William Arnold <[email protected]>
Signed-off-by: William Arnold <[email protected]>
Signed-off-by: William Arnold <[email protected]>
Signed-off-by: William Arnold <[email protected]>
Aphoh
force-pushed
the
warnold/create-debug-config-endpoint
branch
from
af5dcf5 to
ffbe850
Compare
Signed-off-by: William Arnold <[email protected]> Signed-off-by: Piotr Tarasiewicz <[email protected]>
Signed-off-by: William Arnold <[email protected]>
2 participants
Overview:
For benchmarking automations and debugging, it can be useful to get a snapshot of each worker's config/environment. This can aid in debugging performance discrepancies across deployments and make it easier to automate benchmark visualization which requires precise descriptions of node configurations.
Details:
components/commonpackage, to allow for shared code across each dynamo engine component. I didn't see any other place for this, so I made one here.pyrightconfig.jsonso that lsps can correctly pick up each of the components.$PYTHONPATH--dump-config-toflag to trtllm, sglang, vllm, frontend. When specified this dumps config, environment, system setup, hostname, etc all to a json file. When not specified, it gets logged at verbose level.dynamo.sglang.Config) may need custom serialization. This usesfunctools'ssingledispatchmethod to allow any class to be correctly converted to a dict before json serialization.Where should the reviewer start?
components/common/src/dynamo/common/config_dumpcomponents/backends/sglang/src/dynamo/sglang/main.pycomponents/backends/vllm/src/dynamo/vllm/main.pycomponents/backends/trtllm/src/dynamo/trtllm/main.pySummary by CodeRabbit
New Features
Chores