fix: correct defects surfaced by a docstring audit - #1243
Open
ajcasagrande wants to merge 1 commit into
Open
Conversation
A docstring audit surfaced places where a comment disagreed with the code because the CODE was wrong. Each was reproduced with a failing test before being fixed; reported issues that did not reproduce were left alone. User-facing defects: - PromptGenerator wraparound re-appended at most one corpus, silently returning fewer than num_tokens whenever num_tokens > 2 * corpus_size. Now loops; byte-identical for inputs that wrapped at most once. - huggingface_generate parsed responses by the global endpoint.streaming flag rather than the payload shape, silently dropping a streamed token whenever a per-request stream override disagreed. Now dispatches on payload shape, matching the sibling endpoints. - ServerMetricsRecord.endpoint_latency_ns is int | None and data_collector passes None when no aiohttp trace timing exists, but SlimRecord required a non-optional int, so to_slim() raised ValidationError on a live path. SlimRecord's field is now optional; the sole consumer already guards None, and a synthetic 0 would have skewed latency stats. - get_level_number raised AttributeError for every string level name. - Custom GPU telemetry fields colliding on a derived internal_name emitted duplicate metrics, including collisions with built-in defaults. - Histogram timeslices starting before the first sample were emitted with a fabricated zero baseline instead of being skipped. - network_latency shutdown slept on Environment.SERVER_METRICS.SHUTDOWN_DELAY; its own settings class had no such field. Added SHUTDOWN_DELAY to _NetworkLatencySettings and regenerated the env-var docs. - An operator-mode rejection message pointed at docs/kubernetes/sweeps.md, which does not exist. Its test asserts every docs path named in the message resolves on disk, so the next stale pointer fails rather than shipping. Robustness and correctness: - cancel_all_tasks accepted a timeout it never used and never awaited the tasks it cancelled. Now bounded-waits, excluding the current task so a call from inside a managed task cannot deadlock. - get_typed_metadata silently returned raw dicts for five categories missing from a hand-maintained mapping. The mapping is now derived from categories.yaml so it cannot drift again, and its test enumerates the declaring categories from the registry rather than a hardcoded list. - Rankings composer skipped _finalize_conversations, so system and user-context prompts never reached the composed conversations. Note this makes the Conversation objects consistent with every sibling composer but does not change the rankings wire payload, which reads only query/passages. - sagemaker loader skipped per-load state resets, reusing the previous file's trace_id across back-to-back loads. - Empty-file and unsupported-format paths returned None or raised UnboundLocalError into consumers expecting neither. - Topic-only ZMQ frames fed an empty payload to the decoder. - Unguarded all_nodes_view dereference before mount. - platform.system() replaced with IS_MACOS per project convention. Not changed, reported instead: three socket-option constants in http_defaults that are dead rather than missing a setsockopt (SO_LINGER takes a struct linger; the other two only affect bind() on a socket that is never bound), and a sticky_router pop() ordering hazard that is unreachable today but is one refactor away from routing a credit to None. Verified: 17730 unit tests pass, ruff clean, generated CLI and env-var docs in sync, plugin schemas validate. Signed-off-by: Anthony Casagrande <acasagrande@nvidia.com>
ajcasagrande
requested review from
FrankD412,
debermudez,
ganeshku1,
ilana-n,
lkomali and
matthewkotila
as code owners
August 1, 2026 15:25
Contributor
Author
|
Companion: #1242 carries the comment-only half. No files in common; either order is fine. |
This was referenced Aug 1, 2026
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@224533bd6420223537ae0df5f4a31373989daebfRecommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@224533bd6420223537ae0df5f4a31373989daebfLast updated for commit: |
Contributor
WalkthroughThe pull request updates runtime configuration, task cancellation, dataset handling, endpoint parsing, telemetry metadata, plugin resolution, server-metrics export, and related regression tests and documentation. ChangesRuntime and data-path corrections
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/aiperf/dataset/generator/audio.py (1)
180-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
AudioFormatwithout.value.Line 181 accesses a string enum through
.value. Buildsupported_formatsfrom the direct string enum representation. Preservewavandmp3in the error message.As per coding guidelines, “Use string-based enums through
MessageType.Xdirectly; never use.value.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aiperf/dataset/generator/audio.py` around lines 180 - 185, Update the unsupported-format branch in the audio generator to build supported_formats from each AudioFormat member directly rather than accessing .value, while preserving the existing sorted list and ensuring wav and mp3 remain in the error message.Source: Coding guidelines
tests/unit/dataset/composer/test_custom_composer.py (1)
569-580: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type hints to the changed test methods.
Each changed test method lacks type hints for its fixture parameters and its
Nonereturn value.
tests/unit/dataset/composer/test_custom_composer.py#L569-L580: Add fixture parameter types and-> None.tests/unit/dataset/composer/test_custom_composer.py#L582-L593: Add fixture parameter types and-> None.tests/unit/dataset/composer/test_synthetic_rankings_composer.py#L112-L123: Add fixture parameter types and-> None.tests/unit/dataset/composer/test_synthetic_rankings_composer.py#L126-L137: Add fixture parameter types and-> None.tests/unit/dataset/generator/test_audio_generator.py#L294-L301: Add-> None.tests/unit/dataset/generator/test_prompt_generator.py#L450-L466: Add fixture and mock parameter types and-> None.tests/unit/dataset/generator/test_prompt_generator.py#L468-L481: Add fixture parameter types and-> None.tests/unit/dataset/loader/test_can_load.py#L405-L412: Add fixture parameter types and-> None.tests/unit/dataset/loader/test_sagemaker_data_capture.py#L905-L914: Addtmp_pathtype and-> None.tests/unit/dataset/loader/test_sagemaker_data_capture.py#L916-L923: Addtmp_pathtype and-> None.As per coding guidelines, “Add type hints to every function parameter and return value.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/dataset/composer/test_custom_composer.py` around lines 569 - 580, Annotate every parameter and return value in the affected tests: tests/unit/dataset/composer/test_custom_composer.py lines 569-580 and 582-593; tests/unit/dataset/composer/test_synthetic_rankings_composer.py lines 112-123 and 126-137; tests/unit/dataset/generator/test_audio_generator.py lines 294-301; tests/unit/dataset/generator/test_prompt_generator.py lines 450-466 and 468-481; tests/unit/dataset/loader/test_can_load.py lines 405-412; and tests/unit/dataset/loader/test_sagemaker_data_capture.py lines 905-914 and 916-923. Use the existing fixture and mock annotation conventions in each test module, add the appropriate type for tmp_path and other fixture parameters, and declare each test method with a None return type.Source: Coding guidelines
tests/unit/endpoints/test_huggingface_generate.py (2)
108-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the prescribed parametrization format.
The new decorators use raw booleans and place
# fmt: skipon the decorator line. The test guidelines requirefrom pytest import paramand the formatter directive on the closing)line.As per coding guidelines, use
param(...)for parametrized cases and place# fmt: skipon the closing)line.Proposed change
+from pytest import param + - `@pytest.mark.parametrize`("global_streaming", [True, False]) # fmt: skip + `@pytest.mark.parametrize`( + "global_streaming", + [param(True), param(False)], + ) # fmt: skip🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/endpoints/test_huggingface_generate.py` around lines 108 - 134, Update both parametrization decorators in test_parse_response_stream_event_shape_calls_streaming and test_parse_response_full_body_shape_calls_non_streaming to use pytest.param via the prescribed from pytest import param import, and move # fmt: skip to the closing parenthesis line of each decorator. Preserve the existing True and False test cases.Source: Coding guidelines
109-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type hints to each new test method.
The same type-hint gap appears across these changed test methods. Add annotations to fixture parameters and
-> Noneto each method.
tests/unit/endpoints/test_huggingface_generate.py#L109-L167: annotate each new test method's parameters and return value.tests/unit/endpoints/test_solido_rag.py#L301-L303: annotateendpointand add-> None.tests/unit/ui/test_realtime_telemetry_dashboard.py#L656-L663: annotate the method parameters and add-> None.tests/unit/zmq/test_sub_client.py#L195-L208: annotate the fixture parameters and add-> None.As per coding guidelines, every Python function parameter and return value must have a type hint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/endpoints/test_huggingface_generate.py` around lines 109 - 167, Annotate every changed test method with type hints: add appropriate fixture parameter types and -> None in tests/unit/endpoints/test_huggingface_generate.py:109-167, tests/unit/endpoints/test_solido_rag.py:301-303, tests/unit/ui/test_realtime_telemetry_dashboard.py:656-663, and tests/unit/zmq/test_sub_client.py:195-208. Preserve each test’s existing behavior and use the established fixture types in the surrounding test modules.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aiperf/common/mixins/task_manager_mixin.py`:
- Around line 59-73: Update cancel_all_tasks to resolve the current task before
the cancellation loop and exclude it from both cancellation and asyncio.wait()
via the existing current-task comparison. Preserve sibling cancellation and
timeout handling, and add a regression test where a managed task invokes
cancel_all_tasks while a sibling completes its cancellation cleanup.
In `@src/aiperf/ui/dashboard/realtime_telemetry_dashboard.py`:
- Around line 311-315: Update the telemetry handler around all_nodes_view and
compose so non-empty metrics received before mounting are replayed after the
view is created. Track an explicit rendered-state sentinel rather than using
self.metrics to decide whether visibility and rendering have occurred, ensuring
the mounted view becomes visible and receives the cached list. Add a regression
test in test_realtime_telemetry_dashboard.py covering a non-empty pre-compose
metrics list.
In `@tests/unit/gpu_telemetry/test_metrics_config.py`:
- Around line 458-483: Add the explicit -> None return annotation to both test
methods, test_build_custom_metrics_from_csv_colliding_internal_names_dedupes and
test_build_custom_metrics_from_csv_collision_with_default_internal_name_skipped,
without changing their existing test behavior.
In `@tests/unit/server_metrics/test_timeslices.py`:
- Around line 1196-1220: Add the missing return annotation to
test_histogram_timeslices_slices_before_first_sample_are_skipped, declaring that
the test returns None. Leave the test logic unchanged.
---
Nitpick comments:
In `@src/aiperf/dataset/generator/audio.py`:
- Around line 180-185: Update the unsupported-format branch in the audio
generator to build supported_formats from each AudioFormat member directly
rather than accessing .value, while preserving the existing sorted list and
ensuring wav and mp3 remain in the error message.
In `@tests/unit/dataset/composer/test_custom_composer.py`:
- Around line 569-580: Annotate every parameter and return value in the affected
tests: tests/unit/dataset/composer/test_custom_composer.py lines 569-580 and
582-593; tests/unit/dataset/composer/test_synthetic_rankings_composer.py lines
112-123 and 126-137; tests/unit/dataset/generator/test_audio_generator.py lines
294-301; tests/unit/dataset/generator/test_prompt_generator.py lines 450-466 and
468-481; tests/unit/dataset/loader/test_can_load.py lines 405-412; and
tests/unit/dataset/loader/test_sagemaker_data_capture.py lines 905-914 and
916-923. Use the existing fixture and mock annotation conventions in each test
module, add the appropriate type for tmp_path and other fixture parameters, and
declare each test method with a None return type.
In `@tests/unit/endpoints/test_huggingface_generate.py`:
- Around line 108-134: Update both parametrization decorators in
test_parse_response_stream_event_shape_calls_streaming and
test_parse_response_full_body_shape_calls_non_streaming to use pytest.param via
the prescribed from pytest import param import, and move # fmt: skip to the
closing parenthesis line of each decorator. Preserve the existing True and False
test cases.
- Around line 109-167: Annotate every changed test method with type hints: add
appropriate fixture parameter types and -> None in
tests/unit/endpoints/test_huggingface_generate.py:109-167,
tests/unit/endpoints/test_solido_rag.py:301-303,
tests/unit/ui/test_realtime_telemetry_dashboard.py:656-663, and
tests/unit/zmq/test_sub_client.py:195-208. Preserve each test’s existing
behavior and use the established fixture types in the surrounding test modules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cb8ba3b9-ce36-4fcc-bf0b-9165fc7fff14
📒 Files selected for processing (41)
docs/environment-variables.mdsrc/aiperf/cli_runner/_multi_run.pysrc/aiperf/cli_runner/_process_setup.pysrc/aiperf/common/aiperf_logger.pysrc/aiperf/common/environment.pysrc/aiperf/common/mixins/task_manager_mixin.pysrc/aiperf/common/models/server_metrics_models.pysrc/aiperf/common/models/telemetry_models.pysrc/aiperf/common/protocols.pysrc/aiperf/dataset/composer/custom.pysrc/aiperf/dataset/composer/synthetic_rankings.pysrc/aiperf/dataset/generator/audio.pysrc/aiperf/dataset/generator/prompt.pysrc/aiperf/dataset/loader/sagemaker_data_capture.pysrc/aiperf/endpoints/huggingface_generate.pysrc/aiperf/endpoints/solido_rag.pysrc/aiperf/gpu_telemetry/metrics_config.pysrc/aiperf/network_latency/manager.pysrc/aiperf/plugin/plugins.pysrc/aiperf/server_metrics/export_stats.pysrc/aiperf/server_metrics/parquet_exporter.pysrc/aiperf/ui/dashboard/realtime_telemetry_dashboard.pysrc/aiperf/zmq/sub_client.pytests/unit/cli_runner/test_multi_run_operator_message.pytests/unit/cli_runner/test_process_setup_platform.pytests/unit/common/test_docstring_audit_bugs.pytests/unit/dataset/composer/test_custom_composer.pytests/unit/dataset/composer/test_synthetic_rankings_composer.pytests/unit/dataset/generator/test_audio_generator.pytests/unit/dataset/generator/test_prompt_generator.pytests/unit/dataset/loader/test_can_load.pytests/unit/dataset/loader/test_sagemaker_data_capture.pytests/unit/endpoints/test_huggingface_generate.pytests/unit/endpoints/test_solido_rag.pytests/unit/gpu_telemetry/test_metrics_config.pytests/unit/network_latency/test_manager.pytests/unit/plugin/test_orchestrator_categories.pytests/unit/server_metrics/test_parquet_exporter.pytests/unit/server_metrics/test_timeslices.pytests/unit/ui/test_realtime_telemetry_dashboard.pytests/unit/zmq/test_sub_client.py
Comment on lines
+59
to
+73
| # Never await ourselves: cancel_all_tasks can be invoked from inside one of | ||
| # the managed tasks, and awaiting the current task would deadlock. | ||
| current = asyncio.current_task() | ||
| pending = [ | ||
| task for task in task_list if not task.done() and task is not current | ||
| ] | ||
| if not pending: | ||
| return | ||
|
|
||
| _, still_pending = await asyncio.wait(pending, timeout=timeout) | ||
| if still_pending: | ||
| self.warning( | ||
| lambda: f"{len(still_pending)} task(s) did not finish within " | ||
| f"{timeout}s of cancellation; abandoning them." | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not cancel the calling task.
The loop cancels every task before Lines 61-64 exclude the current task from asyncio.wait(). If a managed task calls cancel_all_tasks(), its cancellation is delivered at Line 68. The method then exits before it waits for sibling cleanup.
Resolve current before the cancellation loop. Exclude it from both cancellation and waiting. Add a regression test where a managed task calls cancel_all_tasks() and a sibling task performs cancellation cleanup.
Proposed fix
+ current = asyncio.current_task()
task_list = list(self.tasks)
for task in task_list:
- task.cancel()
+ if task is not current:
+ task.cancel()
- current = asyncio.current_task()
pending = [
task for task in task_list if not task.done() and task is not current
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Never await ourselves: cancel_all_tasks can be invoked from inside one of | |
| # the managed tasks, and awaiting the current task would deadlock. | |
| current = asyncio.current_task() | |
| pending = [ | |
| task for task in task_list if not task.done() and task is not current | |
| ] | |
| if not pending: | |
| return | |
| _, still_pending = await asyncio.wait(pending, timeout=timeout) | |
| if still_pending: | |
| self.warning( | |
| lambda: f"{len(still_pending)} task(s) did not finish within " | |
| f"{timeout}s of cancellation; abandoning them." | |
| ) | |
| current = asyncio.current_task() | |
| task_list = list(self.tasks) | |
| for task in task_list: | |
| if task is not current: | |
| task.cancel() | |
| # Never await ourselves: cancel_all_tasks can be invoked from inside one of | |
| # the managed tasks, and awaiting the current task would deadlock. | |
| pending = [ | |
| task for task in task_list if not task.done() and task is not current | |
| ] | |
| if not pending: | |
| return | |
| _, still_pending = await asyncio.wait(pending, timeout=timeout) | |
| if still_pending: | |
| self.warning( | |
| lambda: f"{len(still_pending)} task(s) did not finish within " | |
| f"{timeout}s of cancellation; abandoning them." | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiperf/common/mixins/task_manager_mixin.py` around lines 59 - 73, Update
cancel_all_tasks to resolve the current task before the cancellation loop and
exclude it from both cancellation and asyncio.wait() via the existing
current-task comparison. Preserve sibling cancellation and timeout handling, and
add a regression test where a managed task invokes cancel_all_tasks while a
sibling completes its cancellation cleanup.
Comment on lines
+311
to
+315
| if self.all_nodes_view is None: | ||
| # Metrics can arrive before compose() runs (or after teardown); the | ||
| # view is created in compose, so there is nothing to update yet. | ||
| self.metrics = metrics | ||
| return |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replay cached telemetry after the view is created.
When a non-empty metrics list reaches Lines [311]-[315] before compose() runs, the handler stores it and returns. compose() creates all_nodes_view but does not render the cached list. A later update also skips the visibility block because self.metrics is already non-empty. The status message can remain visible while the node view remains hidden.
Replay cached metrics after the view is mounted. Track whether the view has been rendered instead of using self.metrics as the first-update sentinel. Add a regression case with a non-empty pre-compose list and verify that the view becomes visible and receives the list. The current test in tests/unit/ui/test_realtime_telemetry_dashboard.py passes only [] and does not cover this path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiperf/ui/dashboard/realtime_telemetry_dashboard.py` around lines 311 -
315, Update the telemetry handler around all_nodes_view and compose so non-empty
metrics received before mounting are replayed after the view is created. Track
an explicit rendered-state sentinel rather than using self.metrics to decide
whether visibility and rendering have occurred, ensuring the mounted view
becomes visible and receives the cached list. Add a regression test in
test_realtime_telemetry_dashboard.py covering a non-empty pre-compose metrics
list.
Comment on lines
+458
to
+483
| def test_build_custom_metrics_from_csv_colliding_internal_names_dedupes(self): | ||
| """Two custom DCGM fields whose derived internal_name collides must not | ||
| both be emitted as metrics — the second one is skipped.""" | ||
| csv_content = """DCGM_FI_DEV_CUSTOM_THING, gauge, Custom Thing (in W) | ||
| DCGM_FI_PROF_CUSTOM_THING, gauge, Custom Thing Prof (in W) | ||
| """ | ||
| with tempfile.NamedTemporaryFile( | ||
| mode="w", suffix=".csv", delete=False, encoding="utf-8" | ||
| ) as f: | ||
| f.write(csv_content) | ||
| csv_path = Path(f.name) | ||
|
|
||
| try: | ||
| loader = MetricsConfigLoader() | ||
| custom_metrics, new_dcgm_mappings = loader.build_custom_metrics_from_csv( | ||
| custom_csv_path=csv_path | ||
| ) | ||
|
|
||
| internal_names = [m[1] for m in custom_metrics] | ||
| assert internal_names == ["nvidia_custom_thing"] | ||
| assert len(internal_names) == len(set(internal_names)) | ||
| assert new_dcgm_mappings == { | ||
| "DCGM_FI_DEV_CUSTOM_THING": "nvidia_custom_thing" | ||
| } | ||
| finally: | ||
| csv_path.unlink() |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add return annotations to both test methods.
Add -> None to test_build_custom_metrics_from_csv_colliding_internal_names_dedupes and test_build_custom_metrics_from_csv_collision_with_default_internal_name_skipped.
As per coding guidelines, “Add type hints to every function parameter and return value.”
Also applies to: 485-509
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/gpu_telemetry/test_metrics_config.py` around lines 458 - 483, Add
the explicit -> None return annotation to both test methods,
test_build_custom_metrics_from_csv_colliding_internal_names_dedupes and
test_build_custom_metrics_from_csv_collision_with_default_internal_name_skipped,
without changing their existing test behavior.
Source: Coding guidelines
Comment on lines
+1196
to
+1220
| def test_histogram_timeslices_slices_before_first_sample_are_skipped(self): | ||
| ts = ServerMetricsTimeSeries() | ||
| # First sample lands at t=3s; the filter starts at t=0. | ||
| add_histogram_snapshots( | ||
| ts, | ||
| "latency", | ||
| [ | ||
| (3 * NANOS_PER_SECOND, hist({"1.0": 1.0, "+Inf": 2.0}, 20.0, 2.0)), | ||
| (4 * NANOS_PER_SECOND, hist({"1.0": 2.0, "+Inf": 4.0}, 40.0, 4.0)), | ||
| (5 * NANOS_PER_SECOND, hist({"1.0": 3.0, "+Inf": 6.0}, 60.0, 6.0)), | ||
| ], | ||
| ) | ||
|
|
||
| slices = _compute_histogram_timeslices( | ||
| get_histogram(ts, "latency"), | ||
| slice_duration=1.0, | ||
| time_filter=make_time_filter(start_ns=0, end_ns=5 * NANOS_PER_SECOND), | ||
| ) | ||
|
|
||
| assert slices is not None | ||
| # [0-1), [1-2), [2-3) all end before the first sample at t=3s. | ||
| assert all(s.start_ns >= 3 * NANOS_PER_SECOND for s in slices), ( | ||
| f"Slices before first sample were emitted: " | ||
| f"{[(s.start_ns, s.count) for s in slices]}" | ||
| ) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the missing return type.
Line 1196 defines a new test without a return annotation. Add -> None.
Proposed fix
- def test_histogram_timeslices_slices_before_first_sample_are_skipped(self):
+ def test_histogram_timeslices_slices_before_first_sample_are_skipped(self) -> None:As per coding guidelines, “Add type hints to every function parameter and return value.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_histogram_timeslices_slices_before_first_sample_are_skipped(self): | |
| ts = ServerMetricsTimeSeries() | |
| # First sample lands at t=3s; the filter starts at t=0. | |
| add_histogram_snapshots( | |
| ts, | |
| "latency", | |
| [ | |
| (3 * NANOS_PER_SECOND, hist({"1.0": 1.0, "+Inf": 2.0}, 20.0, 2.0)), | |
| (4 * NANOS_PER_SECOND, hist({"1.0": 2.0, "+Inf": 4.0}, 40.0, 4.0)), | |
| (5 * NANOS_PER_SECOND, hist({"1.0": 3.0, "+Inf": 6.0}, 60.0, 6.0)), | |
| ], | |
| ) | |
| slices = _compute_histogram_timeslices( | |
| get_histogram(ts, "latency"), | |
| slice_duration=1.0, | |
| time_filter=make_time_filter(start_ns=0, end_ns=5 * NANOS_PER_SECOND), | |
| ) | |
| assert slices is not None | |
| # [0-1), [1-2), [2-3) all end before the first sample at t=3s. | |
| assert all(s.start_ns >= 3 * NANOS_PER_SECOND for s in slices), ( | |
| f"Slices before first sample were emitted: " | |
| f"{[(s.start_ns, s.count) for s in slices]}" | |
| ) | |
| def test_histogram_timeslices_slices_before_first_sample_are_skipped(self) -> None: | |
| ts = ServerMetricsTimeSeries() | |
| # First sample lands at t=3s; the filter starts at t=0. | |
| add_histogram_snapshots( | |
| ts, | |
| "latency", | |
| [ | |
| (3 * NANOS_PER_SECOND, hist({"1.0": 1.0, "+Inf": 2.0}, 20.0, 2.0)), | |
| (4 * NANOS_PER_SECOND, hist({"1.0": 2.0, "+Inf": 4.0}, 40.0, 4.0)), | |
| (5 * NANOS_PER_SECOND, hist({"1.0": 3.0, "+Inf": 6.0}, 60.0, 6.0)), | |
| ], | |
| ) | |
| slices = _compute_histogram_timeslices( | |
| get_histogram(ts, "latency"), | |
| slice_duration=1.0, | |
| time_filter=make_time_filter(start_ns=0, end_ns=5 * NANOS_PER_SECOND), | |
| ) | |
| assert slices is not None | |
| # [0-1), [1-2), [2-3) all end before the first sample at t=3s. | |
| assert all(s.start_ns >= 3 * NANOS_PER_SECOND for s in slices), ( | |
| f"Slices before first sample were emitted: " | |
| f"{[(s.start_ns, s.count) for s in slices]}" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/server_metrics/test_timeslices.py` around lines 1196 - 1220, Add
the missing return annotation to
test_histogram_timeslices_slices_before_first_sample_are_skipped, declaring that
the test returns None. Leave the test logic unchanged.
Source: Coding guidelines
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
debermudez
approved these changes
Aug 3, 2026
debermudez
left a comment
debermudez
left a comment
Contributor
There was a problem hiding this comment.
Review summary
Solid, well-structured set of fixes. Every one of the 10+ defects is correctly diagnosed and fixed, and each has a regression test that exercises the failure mode rather than just the happy path. The cancel_all_tasks await fix and the get_typed_metadata registry derivation are particularly nice. Two things to address:
- Minor:
time.sleep(0.5)intest_cancel_all_tasks_uncancellable_task_returns_within_timeout— CLAUDE.md forbidstime.sleep. The intent is sound (simulating an uncancellable thread), but athreading.Eventavoids the 0.5s real wall-clock cost and the daemon thread leak. - Cleanup:
src/aiperf/dataset/generator/video.py:46still callsplatform.system()raw — one line that could be fixed here since the PR is already touching this convention in_process_setup.py.
Happy to approve once those two are cleaned up.
| async def _uncancellable() -> None: | ||
| # to_thread cannot be interrupted by cancellation until the thread returns. | ||
| await asyncio.to_thread(time.sleep, 0.5) | ||
|
|
Contributor
There was a problem hiding this comment.
time.sleep violates CLAUDE.md — a threading.Event avoids the real 0.5s cost.
The intent is correct: asyncio.to_thread can't be interrupted by asyncio cancellation until the thread returns, so this genuinely simulates an uncancellable task. But time.sleep(0.5) is real wall-clock time — the auto-fixture only mocks asyncio.sleep — and the daemon thread continues sleeping after the test returns.
Alternative that blocks without sleeping:
async def _uncancellable() -> None:
done = threading.Event()
try:
await asyncio.to_thread(done.wait)
finally:
done.set()The finally fires when the coroutine is cancelled, which sets the event and lets the thread exit immediately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A docstring audit surfaced places where a comment disagreed with the code because the code was wrong. This PR carries those fixes — 41 files, each defect reproduced with a failing test before being fixed.
Companion to #1242 (the comment-only half). The two touch no files in common and can merge in either order.
Reports that did not reproduce were left alone rather than "fixed" — several turned out to be intentional, and two would have caused regressions if changed (see below).
Defects fixed
PromptGeneratorwraparound re-appended at most one corpusnum_tokenswhenevernum_tokens > 2 * corpus_sizehuggingface_generatedispatched on the globalendpoint.streamingflag, not payload shapeSlimRecord.endpoint_latency_nsrequired a non-optionalintto_slim()raisedValidationErroron a live path —data_collectorpassesNonewhen no aiohttp trace timing existsget_level_numberdidgetattr(cls, level.upper())AttributeErrorfor every string level nameinternal_namenetwork_latencyshutdown slept onEnvironment.SERVER_METRICS.SHUTDOWN_DELAYget_typed_metadata's hand-maintained category mappingcategories.yamlso it cannot drift againcancel_all_tasksaccepted atimeoutit never useddocs/kubernetes/sweeps.mdAlso: rankings composer skipped
_finalize_conversations; sagemaker loader skipped per-load state resets, reusing the previous file'strace_id; empty-file and unsupported-format paths returnedNoneor raisedUnboundLocalError; topic-only ZMQ frames fed an empty payload to the decoder; unguardedall_nodes_viewdereference before mount;platform.system()replaced withIS_MACOSper project convention.Choices worth a reviewer's attention
SlimRecord.endpoint_latency_nsmade optional rather than defaulting to0into_slim()— the sole consumer already guardsNone, and a synthetic zero would silently skew latency statistics._compute_histogram_stats' fallback exists because whole-range stats must return something.cancel_all_tasksexcludes the current task from its wait set — it is called from inside managed tasks on the shutdown path, and awaiting self would deadlock. A genuinely uncancellable task is used in the test to prove the bounded return.base_rankings_endpoint.format_payloadreads onlyquery/passages. This makes theConversationobjects and exportedinputs.jsonconsistent with every sibling composer; carrying a system prompt in a rankings request would need an endpoint-side change.docs/path named in the message resolves on disk. Both catch the next instance of the bug they guard rather than the one instance found.Deliberately not changed
http_defaults.pyare dead, not missing asetsockopt—SO_LINGERtakes astruct linger(not an int), andSO_REUSEADDR/SO_REUSEPORTonly affectbind()on a socket that is never bound. Adding the calls would change connection behavior at scale for no benefit.sticky_routerpop()ordering hazard is unreachable today (noawaitbetween selection and tracking, single-threaded asyncio) but is one refactor away from routing a credit toNone. Flagged rather than changed on a hot path.Verification
uv run pytest tests/unit/ -n auto→ 17730 passed, 0 failedruff check/ruff format --checkclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation