-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Enable multi-concurrency scans #125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -273,14 +273,36 @@ def _build_config_from_cli( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Raises: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| InputValidationError: If required params missing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Parse concurrency argument (can be single value or comma-separated list) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| concurrency_value: int | list[int] | None = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if concurrency_str := getattr(args, "concurrency", None): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(concurrency_str, int): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| concurrency_value = concurrency_str | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif "," in concurrency_str: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Parse comma-separated list | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| concurrency_value = [int(c.strip()) for c in concurrency_str.split(",")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except ValueError as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise InputValidationError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"Invalid concurrency value '{concurrency_str}': all values must be integers" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) from e | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Parse single integer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| concurrency_value = int(concurrency_str) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except ValueError as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise InputValidationError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"Invalid concurrency value '{concurrency_str}': must be an integer" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) from e | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+278
to
+296
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic for parsing the
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Determine load pattern (CLI override or mode default) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if load_pattern_arg := getattr(args, "load_pattern", None): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| load_pattern_type = LoadPatternType(load_pattern_arg) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| match benchmark_mode: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case "offline": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| load_pattern_type = LoadPatternType.MAX_THROUGHPUT | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case "online" if getattr(args, "concurrency", None): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case "online" if concurrency_value is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| load_pattern_type = LoadPatternType.CONCURRENCY | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case "online": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| load_pattern_type = LoadPatternType.POISSON | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -309,7 +331,7 @@ def _build_config_from_cli( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| load_pattern=LoadPattern( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type=load_pattern_type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_qps=getattr(args, "target_qps", None), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_concurrency=getattr(args, "concurrency", None), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_concurrency=concurrency_value, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| runtime=RuntimeConfig( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| min_duration_ms=args.duration * 1000 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -359,9 +381,90 @@ def _run_benchmark( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| test_mode: TestMode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| benchmark_mode: TestType | None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Execute the actual benchmark with full lifecycle management. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Execute benchmark(s) - either single run or multiple runs for different concurrency values. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| This function handles the top-level orchestration: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 1. If target_concurrency is a single value or not applicable, run one benchmark | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 2. If target_concurrency is a list, run multiple benchmarks sequentially | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 3. Each benchmark run gets its own subdirectory (concurrency_{value}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 4. Resources are fully cleaned up between runs to ensure isolation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config: Validated BenchmarkConfig (immutable Pydantic model). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| collect_responses: Whether to store full response text. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| test_mode: What to collect - PERF, ACC, or BOTH. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| benchmark_mode: Execution mode - OFFLINE or ONLINE. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Determine base report directory | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if config.report_dir: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| base_report_dir = Path(config.report_dir) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| base_report_dir = get_default_report_path() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+399
to
+402
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Check if we need to run multiple benchmarks for different concurrency values | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| load_pattern = config.settings.load_pattern | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_concurrency = load_pattern.target_concurrency | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # If concurrency mode with list of values, run multiple benchmarks | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if load_pattern.type == LoadPatternType.CONCURRENCY and isinstance( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_concurrency, list | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"Running {len(target_concurrency)} benchmarks with concurrency values: {target_concurrency}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for i, concurrency_val in enumerate(target_concurrency): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"\n{'='*80}\nBenchmark {i+1}/{len(target_concurrency)}: Concurrency = {concurrency_val}\n{'='*80}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Create subdirectory for this concurrency value | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| concurrency_report_dir = base_report_dir / f"concurrency_{concurrency_val}" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+421
to
+422
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Create subdirectory for this concurrency value | |
| concurrency_report_dir = base_report_dir / f"concurrency_{concurrency_val}" | |
| # Create subdirectory for this concurrency value (include run index to avoid collisions) | |
| concurrency_report_dir = base_report_dir / f"run_{i+1}_concurrency_{concurrency_val}" |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reconstructing nested Pydantic models via model_dump() + manual re-wrapping is hard to read and easy to get wrong as the schema evolves. Prefer using Pydantic’s copy/update utilities (e.g., model_copy(update=...) on BenchmarkConfig, Settings, and LoadPattern) to express “same config except target_concurrency=…” more directly and reduce the risk of losing defaults/aliases or introducing subtle serialization differences.
| config = BenchmarkConfig( | |
| **{ | |
| **config.model_dump(), | |
| "settings": Settings( | |
| **{ | |
| **config.settings.model_dump(), | |
| "load_pattern": LoadPattern( | |
| **{ | |
| **config.settings.load_pattern.model_dump(), | |
| "target_concurrency": concurrency_value, | |
| } | |
| ), | |
| } | |
| ), | |
| config = config.model_copy( | |
| update={ | |
| "settings": config.settings.model_copy( | |
| update={ | |
| "load_pattern": config.settings.load_pattern.model_copy( | |
| update={"target_concurrency": concurrency_value} | |
| ) | |
| } | |
| ) |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This manual rebuild of BenchmarkConfig is quite verbose/error-prone. Since this is a Pydantic v2 model, it’s typically simpler (and less likely to miss fields) to use config.model_copy(update=..., deep=True) to override only settings.load_pattern.target_concurrency.
| # Create a new config with the specific concurrency value | |
| config = BenchmarkConfig( | |
| **{ | |
| **config.model_dump(), | |
| "settings": Settings( | |
| **{ | |
| **config.settings.model_dump(), | |
| "load_pattern": LoadPattern( | |
| **{ | |
| **config.settings.load_pattern.model_dump(), | |
| "target_concurrency": concurrency_value, | |
| } | |
| ), | |
| } | |
| ), | |
| } | |
| # Create a new config with the specific concurrency value by copying and updating | |
| config = config.model_copy( | |
| update={ | |
| "settings": config.settings.model_copy( | |
| update={ | |
| "load_pattern": config.settings.load_pattern.model_copy( | |
| update={"target_concurrency": concurrency_value}, | |
| deep=True, | |
| ) | |
| }, | |
| deep=True, | |
| ) | |
| }, | |
| deep=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method used to update the BenchmarkConfig with a new concurrency_value is quite complex and hard to maintain. It involves deeply nested dictionary unpacking and model reconstruction.
Pydantic's model_copy(update=...) method provides a much cleaner and more robust way to create a modified copy of a frozen model. This would make the code more readable and less prone to errors if the model structure changes.
config = config.model_copy(
update={
"settings": config.settings.model_copy(
update={
"load_pattern": config.settings.load_pattern.model_copy(
update={"target_concurrency": concurrency_value}
)
}
)
}
)| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -25,7 +25,7 @@ | |||||
| from typing import ClassVar | ||||||
|
|
||||||
| import yaml | ||||||
| from pydantic import BaseModel, Field | ||||||
| from pydantic import BaseModel, Field, field_validator | ||||||
|
|
||||||
| from .. import metrics | ||||||
| from .ruleset_base import BenchmarkSuiteRuleset | ||||||
|
|
@@ -266,13 +266,36 @@ class LoadPattern(BaseModel): | |||||
| - max_throughput: target_qps used for calculating total queries (offline, optional with default) | ||||||
| - poisson: target_qps sets scheduler rate (online, required - validated) | ||||||
| - concurrency: issue at fixed target_concurrency (online, required - validated) | ||||||
|
|
||||||
| target_concurrency can be either: | ||||||
| - Single int: Run one benchmark with that concurrency level | ||||||
| - List of ints: Run multiple benchmarks sequentially, one per concurrency level | ||||||
| """ | ||||||
|
|
||||||
| type: LoadPatternType = LoadPatternType.MAX_THROUGHPUT | ||||||
| target_qps: float | None = ( | ||||||
| None # Target QPS - required for poisson pattern, optional otherwise | ||||||
| ) | ||||||
| target_concurrency: int | None = None # For concurrency mode, ignored otherwise | ||||||
| target_concurrency: int | list[int] | None = ( | ||||||
| None # For concurrency mode, ignored otherwise | ||||||
| ) | ||||||
|
|
||||||
| @field_validator("target_concurrency", mode="before") | ||||||
| @classmethod | ||||||
| def validate_target_concurrency(cls, v): | ||||||
| """Validate target_concurrency accepts int or list of ints.""" | ||||||
| if v is None: | ||||||
| return v | ||||||
| # Accept single int | ||||||
| if isinstance(v, int): | ||||||
| return v | ||||||
| # Accept list of ints | ||||||
| if isinstance(v, list): | ||||||
| return v | ||||||
| # Try to convert if it's something else (shouldn't happen with proper YAML) | ||||||
|
||||||
| # Try to convert if it's something else (shouldn't happen with proper YAML) | |
| # Reject any other types (this should not happen with proper YAML) |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validator’s docstring and comments claim it validates “list of ints” and “tries to convert”, but it currently returns any list unchanged (including list[str], list[float], etc.) and performs no conversion. Either (a) actually validate/convert list elements here (e.g., ensure each element is an int and/or coerce string ints if that’s desired), or (b) remove this validator and rely on the later validate_load_pattern logic (to avoid duplicated/partial validation paths).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -379,7 +379,11 @@ def __init__(self, runtime_settings: RuntimeSettings, sample_order_cls): | |
| super().__init__(runtime_settings, sample_order_cls) | ||
| assert runtime_settings.load_pattern is not None | ||
| target_concurrency = runtime_settings.load_pattern.target_concurrency | ||
| if target_concurrency is None or target_concurrency <= 0: | ||
| if ( | ||
| target_concurrency is None | ||
| or (isinstance(target_concurrency, int) and target_concurrency <= 0) | ||
| or (isinstance(target_concurrency, list) and len(target_concurrency) == 0) | ||
| ): | ||
| raise ValueError( | ||
| f"target_concurrency must be > 0 for CONCURRENCY load pattern, got {target_concurrency}" | ||
| ) | ||
|
Comment on lines
+382
to
389
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--concurrencyis defined astype=strin argparse, soconcurrency_strwill never be aninthere. Dropping the deadisinstance(concurrency_str, int)branch will simplify the logic and avoid misleading future readers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems valid?