Skip to content

Conversation

@thameem-abbas
Copy link
Member

@thameem-abbas thameem-abbas commented Oct 28, 2025

This allows the GuideLLM log level to be raised.

Summary by CodeRabbit

  • New Features
    • Added configurable logging level for benchmarks (default: INFO) so users can adjust verbosity.
    • Benchmark runs now propagate the chosen logging level to launched benchmark processes, ensuring consistent console logging.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Walkthrough

Adds a logging_level field to BenchmarkConfig (typed as a Literal) and passes it via the GUIDELLM__LOGGING__CONSOLE_LOG_LEVEL environment variable when launching GuideLLM subprocesses.

Changes

Cohort / File(s) Summary
Configuration extension
auto_tune_vllm/benchmarks/config.py
Added logging_level: Literal["DEBUG","INFO","WARNING","ERROR","CRITICAL"] = "INFO" to BenchmarkConfig and imported Literal from typing. A comment documents its purpose.
Subprocess environment injection
auto_tune_vllm/benchmarks/providers.py
When starting the GuideLLM benchmark, copy the current environment, set GUIDELLM__LOGGING__CONSOLE_LOG_LEVEL from config.logging_level, and pass the updated env to subprocess.Popen. Minor comment/formatting tweaks.

Sequence Diagram

sequenceDiagram
    participant User
    participant BenchmarkConfig
    participant Provider
    participant GuideLLM_Process as Subprocess

    User->>BenchmarkConfig: set logging_level
    BenchmarkConfig->>Provider: provide config
    Provider->>Provider: env_copy = os.environ.copy()
    Provider->>Provider: env_copy["GUIDELLM__LOGGING__CONSOLE_LOG_LEVEL"] = logging_level
    Provider->>GuideLLM_Process: subprocess.Popen(..., env=env_copy)
    GuideLLM_Process->>GuideLLM_Process: read env and apply logging level
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Quick checks: correct Literal import and typing, default value, and comment placement in config.py.
  • Verify environment copy, key name, and that env is passed to subprocess.Popen in providers.py.
  • Ensure no unintended side-effects for other subprocess invocations.

Poem

🐰 I hopped in code to tweak a line,
A logging flag now set by design,
Env whispered low, subprocess obeys,
GuideLLM chats in calmer ways — hooray! 🎉

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Add: Config Var to allow raising GuideLLM logging level" is fully related to the main change in the changeset. The pull request adds a new logging_level configuration field to the BenchmarkConfig dataclass and updates the providers module to use this field to set the GuideLLM logging environment variable. The title clearly and concisely captures this core change, using specific language that describes both the action (adding a config variable) and its purpose (controlling GuideLLM logging level). A teammate reviewing the git history would immediately understand that this PR introduces configurable logging level support.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/add_guidellm_benchmark

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94345cf and acee7d5.

📒 Files selected for processing (1)
  • auto_tune_vllm/benchmarks/config.py (2 hunks)
🧰 Additional context used
🪛 GitHub Check: pyright
auto_tune_vllm/benchmarks/config.py

[warning] 4-4:
This type is deprecated as of Python 3.10; use "| None" instead (reportDeprecated)

🔇 Additional comments (2)
auto_tune_vllm/benchmarks/config.py (2)

4-4: LGTM! Necessary import for type-safe logging configuration.

The Literal import is correctly added to support the new logging_level field with compile-time type checking.


32-34: Well-implemented logging configuration with proper type safety.

The logging_level field is correctly implemented with:

  • Literal type restricting values to Python's standard logging levels
  • Sensible default of "INFO"
  • Clear comment indicating its purpose and usage

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
auto_tune_vllm/benchmarks/config.py (1)

31-33: Consider adding validation or documentation for valid logging levels.

The implementation looks good with a sensible default. To improve robustness, consider either:

  1. Adding a comment documenting valid values (e.g., DEBUG, INFO, WARNING, ERROR)
  2. Using a Literal type hint: logging_level: Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] = "INFO"

This would provide better IDE support and catch invalid values at type-check time.

Example with Literal:

 from dataclasses import dataclass
-from typing import Optional
+from typing import Literal, Optional
     # Logging level for GuideLLM
-    logging_level: str = "INFO"
+    logging_level: Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] = "INFO"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e026768 and 94345cf.

📒 Files selected for processing (2)
  • auto_tune_vllm/benchmarks/config.py (1 hunks)
  • auto_tune_vllm/benchmarks/providers.py (1 hunks)
⏰ 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: pyright
🔇 Additional comments (1)
auto_tune_vllm/benchmarks/providers.py (1)

225-237: LGTM! Clean environment variable handling.

The implementation correctly:

  • Copies the current environment to avoid modifying the parent process
  • Injects the GuideLLM-specific logging configuration
  • Passes the modified environment to the subprocess

This approach properly isolates the subprocess environment changes.

Copy link
Collaborator

@ephoris ephoris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@thameem-abbas thameem-abbas merged commit 5360a7e into main Oct 29, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants