-
Notifications
You must be signed in to change notification settings - Fork 11
feat: use msgspec for faster JSON encoding and decoding #493
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?
Conversation
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@deac901f5795c530d0e56e11f8218f1487b0713eRecommended 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@deac901f5795c530d0e56e11f8218f1487b0713eLast updated for commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
WalkthroughThe codebase migrates from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks✅ Passed checks (3 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 |
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.
Actionable comments posted: 2
🧹 Nitpick comments (11)
CLAUDE.md (1)
81-81: Update retrieved learnings to reflect msgspec migration.The coding guideline has been updated to use msgspec, but the retrieved learnings still reference orjson. Based on learnings, the previous standard was to use orjson. Since this PR intentionally migrates to msgspec, the learnings should be updated to reflect this new standard.
tests/unit/dataset/loader/conftest.py (1)
27-36: Consider clarifying the fallback case.The file mode change to binary ("wb") and the type handling logic are correct for msgspec compatibility. However, the
elsebranch on line 35 is unclear—what type would reach this case that isn'tstrorbytes? Consider either removing this fallback or adding a comment explaining when it would be triggered. If no other types are expected, raising a TypeError would be more explicit.Apply this diff to make the handling more explicit:
- if isinstance(line, str): - f.write(line.encode("utf-8") + b"\n") - elif isinstance(line, bytes): - f.write(line + b"\n") - else: - f.write(line + b"\n") + if isinstance(line, str): + f.write(line.encode("utf-8") + b"\n") + elif isinstance(line, bytes): + f.write(line + b"\n") + else: + raise TypeError(f"Expected str or bytes, got {type(line)}")tests/unit/common/models/test_auto_routed_messages.py (1)
5-5: Routing tests correctly exercise msgspec-encoded inputs; consider aligning docstring with actual input typeUsing
msgspec.json.encodefor thefrom_jsoninputs (plus the derived str/bytes/bytearray variants) is consistent with the new JSON backend and keeps these tests close to real usage. The only mismatch is thattest_json_string_routingnow passes bytes while its docstring advertises a JSON string; either decoding to UTF‑8 before callingfrom_json, or updating the docstring to mention “JSON bytes” would remove that confusion.Please double‑check that
Message.from_jsonandload_json_strare intended to support all four input types (dict, str, bytes, bytearray) with msgspec as the backend, and update the docstring accordingly if bytes are now the primary path.Also applies to: 143-152, 178-185
src/aiperf/common/config/config_validators.py (1)
8-8: Exception handling for msgspec-backed JSON parsing is correct; reconcile with global JSON-backend guidelineCatching
msgspec.DecodeErroraroundload_json_str(input)and re‑raising aValueErrorkeeps the function’s external behavior intact while aligning with the new msgspec JSON backend. The only concern is process-level: repo guidelines currently say to “Always use orjson for JSON operations”, but this module (and others in the PR) now depend on msgspec instead. It would be good to either (a) confirm that msgspec is the new standard and update the guidelines/CL docs, or (b) explicitly document this module as an intentional exception to avoid future confusion.Please verify the current contract of
load_json_str(i.e., that it raisesmsgspec.DecodeErrorfor malformed JSON) and update the coding guidelines or an ADR to reflect msgspec as the canonical JSON backend if that’s now the intended direction. As per coding guidelines, this change currently diverges from the “Always use orjson for JSON operations” rule.Also applies to: 150-157
src/aiperf/common/models/sequence_distribution.py (1)
38-38: JSON distribution parsing correctly wraps msgspec.DecodeError; ensure backend contract stays stableCatching
msgspec.DecodeErrorin_parse_pairs_from_jsonand re‑raising aValueError("Invalid JSON format: ...")is a sensible way to surface malformed distribution specs while delegating actual parsing toload_json_str. This matches the broader move to msgspec, but also hard‑codes knowledge of the JSON backend into this model module, similar toconfig_validators.Please verify that
load_json_strguarantees raisingmsgspec.DecodeError(and not some other exception type) for malformed JSON so this handler remains effective, and consider documenting msgspec as the canonical JSON backend in the project guidelines to avoid future confusion about the “Always use orjson” rule that’s still present in the shared coding guidelines.Also applies to: 361-367
src/aiperf/transports/aiohttp_transport.py (1)
10-10: Payload serialization via msgspec is correct; consider bytes path laterSwitching to
msgspec.json.encode(payload).decode("utf-8")cleanly preserves the existingstrpayload contract toAioHttpClient.post_requestwhile using the new backend.If you later relax
AioHttpClientto acceptbytes, you could skip the.decode("utf-8")here and pass the encoded bytes directly for a tiny efficiency win, but that’s purely optional.Also applies to: 151-153
tests/unit/transports/test_aiohttp_client.py (1)
9-9: End‑to‑end transport tests correctly switched to msgspec serializationUsing
msgspec.json.encode(...).decode("utf-8")for both mocked responses and request bodies brings the tests in line with the transport’s new serialization behavior while preserving thestrAPI expected byAioHttpClient.post_request.If you find yourself adding more such tests, consider a small local helper like
encode_json(obj) -> strto DRY up the encode/decode pattern, but it’s not required here.Also applies to: 326-334, 355-358
src/aiperf/common/messages/base_messages.py (1)
6-7:Message.to_json_bytescorrectly migrated to msgspec; update global JSON guidelineUsing
msgspec.json.encode(self.model_dump(exclude_none=True, mode="json"))here is a clean swap from the previous orjson implementation: callers still get compact JSON bytes withNonefields stripped, and the docstring accurately reflects the new backend and performance intent.Given this central message type now standardizes on msgspec, it would be good to update CLAUDE/coding guidelines that currently say “Always use orjson for JSON operations” so they instead point to msgspec as the preferred JSON backend going forward. Based on learnings, that will keep future changes consistent with this PR’s direction.
Also applies to: 46-61
tests/unit/dataset/loader/test_single_turn.py (1)
4-4: Verifycreate_jsonl_filehandles msgspec-encoded bytes
msgspec.json.encode(...)returnsbytes, whereas most other tests here pass JSONstrliterals intocreate_jsonl_file. This is fine as long as the fixture writes bytes (or normalizes both bytes/str internally); otherwise you may hit type errors when constructing the JSONL file.If
create_jsonl_filestill assumesstr, either update it to accept bothbytesandstr, or decode here with.decode("utf-8")before adding tocontent.Also applies to: 257-273
tests/unit/dataset/loader/test_multi_turn.py (1)
4-4: Ensure dataset JSONL helper supports msgspec-encoded bytes (and mixed bytes/str)All these tests now use
msgspec.json.encode(...), which returnsbytes, and in some cases (test_load_dataset_skips_empty_lines) thecontentlist mixesbytesandstr(the empty-line sentinel).This is only safe if
create_jsonl_filehas been updated to:
- accept both
bytesandstr, and- write them in a compatible way (e.g., binary mode for bytes, or decoding bytes to UTF‑8 before writing as text).
If the fixture still assumes
strand does operations likeline + "\n"in text mode, these tests will fail. Consider either:
- normalizing each encoded payload with
.decode("utf-8")before adding it tocontent, or- explicitly updating
create_jsonl_fileto handlebytes | strrobustly.Also applies to: 126-136, 158-176, 195-203, 223-239, 261-271, 286-301, 322-343, 373-386, 403-414
src/aiperf/endpoints/template_endpoint.py (1)
11-12: msgspec-based decoding looks good; consider asserting object type and reconciling JSON-lib guidelinesSwitching from
orjson.loads(rendered)tomsgspec.json.decode(rendered)and catchingmsgspec.DecodeErroris a valid replacement here, and the error propagation remains clear.Two follow-ups you might consider:
- After decoding, defensively assert the template produced a JSON object before calling
payload.update(...), e.g.if not isinstance(payload, dict): raise ValueError("Template must render a JSON object, got ..."), so non-object templates fail fast with a clearer message.- The current coding guidelines still state “Always use orjson for JSON operations”; if msgspec is now the project-wide standard (as this PR suggests), those guidelines should be updated to avoid contradicting this implementation.
As per coding guidelines, the JSON backend choice should be documented consistently.
Also applies to: 145-153
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
CLAUDE.md(1 hunks)pyproject.toml(1 hunks)src/aiperf/common/config/config_validators.py(2 hunks)src/aiperf/common/config/user_config.py(2 hunks)src/aiperf/common/messages/base_messages.py(3 hunks)src/aiperf/common/mixins/buffered_jsonl_writer_mixin.py(2 hunks)src/aiperf/common/models/record_models.py(3 hunks)src/aiperf/common/models/sequence_distribution.py(2 hunks)src/aiperf/common/utils.py(2 hunks)src/aiperf/endpoints/template_endpoint.py(2 hunks)src/aiperf/transports/aiohttp_transport.py(2 hunks)tests/aiperf_mock_server/config.py(2 hunks)tests/integration/test_gpu_telemetry.py(2 hunks)tests/integration/test_random_generator_canary.py(2 hunks)tests/integration/utils.py(3 hunks)tests/unit/common/messages/test_messages.py(11 hunks)tests/unit/common/mixins/test_buffered_jsonl_writer_mixin.py(2 hunks)tests/unit/common/models/test_auto_routed_messages.py(3 hunks)tests/unit/common/models/test_sequence_distribution.py(1 hunks)tests/unit/dataset/loader/conftest.py(1 hunks)tests/unit/dataset/loader/test_multi_turn.py(10 hunks)tests/unit/dataset/loader/test_single_turn.py(2 hunks)tests/unit/dataset/test_dataset_manager_inputs_json.py(7 hunks)tests/unit/exporters/test_metrics_json_exporter.py(12 hunks)tests/unit/exporters/test_timeslice_metrics_json_exporter.py(8 hunks)tests/unit/post_processors/test_raw_record_writer_processor.py(5 hunks)tests/unit/post_processors/test_record_export_results_processor.py(5 hunks)tests/unit/post_processors/test_telemetry_export_results_processor.py(14 hunks)tests/unit/server/test_config.py(1 hunks)tests/unit/transports/test_aiohttp_client.py(3 hunks)tests/unit/transports/test_aiohttp_transport.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use async/await for all I/O operations; never use time.sleep() or blocking calls
Always use orjson for JSON operations: orjson.loads(s) and orjson.dumps(d)
All functions must have type hints on parameters and return types
Use Python 3.10+ union syntax (|) instead of typing.Union; use match/case for pattern matching; use @DataClass(slots=True)
Files:
tests/unit/common/mixins/test_buffered_jsonl_writer_mixin.pytests/unit/server/test_config.pysrc/aiperf/endpoints/template_endpoint.pysrc/aiperf/common/utils.pytests/unit/exporters/test_metrics_json_exporter.pytests/unit/post_processors/test_telemetry_export_results_processor.pytests/unit/transports/test_aiohttp_transport.pytests/integration/test_random_generator_canary.pytests/unit/post_processors/test_raw_record_writer_processor.pytests/unit/dataset/loader/test_multi_turn.pytests/unit/exporters/test_timeslice_metrics_json_exporter.pysrc/aiperf/common/models/record_models.pytests/integration/test_gpu_telemetry.pytests/integration/utils.pytests/unit/common/models/test_sequence_distribution.pysrc/aiperf/common/config/user_config.pysrc/aiperf/common/mixins/buffered_jsonl_writer_mixin.pytests/unit/common/messages/test_messages.pytests/unit/post_processors/test_record_export_results_processor.pysrc/aiperf/transports/aiohttp_transport.pytests/unit/dataset/loader/conftest.pytests/unit/dataset/loader/test_single_turn.pytests/unit/dataset/test_dataset_manager_inputs_json.pytests/unit/transports/test_aiohttp_client.pysrc/aiperf/common/models/sequence_distribution.pytests/unit/common/models/test_auto_routed_messages.pysrc/aiperf/common/messages/base_messages.pytests/aiperf_mock_server/config.pysrc/aiperf/common/config/config_validators.py
**/*test*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test files must use pytest with fixtures, helpers, and @pytest.mark.parametrize; import statements at the top; use # fmt: skip for long parameterize blocks
Files:
tests/unit/common/mixins/test_buffered_jsonl_writer_mixin.pytests/unit/server/test_config.pytests/unit/exporters/test_metrics_json_exporter.pytests/unit/post_processors/test_telemetry_export_results_processor.pytests/unit/transports/test_aiohttp_transport.pytests/integration/test_random_generator_canary.pytests/unit/post_processors/test_raw_record_writer_processor.pytests/unit/dataset/loader/test_multi_turn.pytests/unit/exporters/test_timeslice_metrics_json_exporter.pytests/integration/test_gpu_telemetry.pytests/unit/common/models/test_sequence_distribution.pytests/unit/common/messages/test_messages.pytests/unit/post_processors/test_record_export_results_processor.pytests/unit/dataset/loader/conftest.pytests/unit/dataset/loader/test_single_turn.pytests/unit/dataset/test_dataset_manager_inputs_json.pytests/unit/transports/test_aiohttp_client.pytests/unit/common/models/test_auto_routed_messages.py
**/models/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Data models must inherit from AIPerfBaseModel and use Field(description='...') for all Pydantic fields
Files:
src/aiperf/common/models/record_models.pytests/unit/common/models/test_sequence_distribution.pysrc/aiperf/common/models/sequence_distribution.pytests/unit/common/models/test_auto_routed_messages.py
**/config/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Configuration classes must inherit from BaseConfig and include Field(description='...') for all Pydantic fields
Files:
src/aiperf/common/config/user_config.pysrc/aiperf/common/config/config_validators.py
**/messages/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Messages must be properly defined with message_type and Field(description=...) for all fields in message classes
Files:
tests/unit/common/messages/test_messages.pysrc/aiperf/common/messages/base_messages.py
**/conftest.py
📄 CodeRabbit inference engine (CLAUDE.md)
Auto-fixtures in tests should mock time, set RNG=42, and reset singletons
Files:
tests/unit/dataset/loader/conftest.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: ai-dynamo/aiperf PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T00:08:56.784Z
Learning: Applies to **/*.py : Always use orjson for JSON operations: orjson.loads(s) and orjson.dumps(d)
📚 Learning: 2025-11-25T00:08:56.784Z
Learnt from: CR
Repo: ai-dynamo/aiperf PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T00:08:56.784Z
Learning: Applies to **/{*service,*component}.py : Use expensive log calls with lambda: self.debug(lambda: f'...'); use direct strings for simple logs: self.info('...')
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-25T00:08:56.784Z
Learnt from: CR
Repo: ai-dynamo/aiperf PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T00:08:56.784Z
Learning: Applies to **/*.py : Always use orjson for JSON operations: orjson.loads(s) and orjson.dumps(d)
Applied to files:
CLAUDE.mdsrc/aiperf/common/utils.pytests/unit/post_processors/test_raw_record_writer_processor.py
🧬 Code graph analysis (7)
tests/unit/transports/test_aiohttp_transport.py (1)
tests/unit/transports/conftest.py (1)
model_endpoint_non_streaming(274-276)
tests/unit/post_processors/test_raw_record_writer_processor.py (1)
src/aiperf/common/models/record_models.py (1)
RawRecordInfo(896-930)
tests/unit/common/messages/test_messages.py (2)
src/aiperf/common/tokenizer.py (1)
decode(98-113)src/aiperf/common/messages/base_messages.py (1)
to_json_bytes(46-60)
tests/unit/dataset/loader/test_single_turn.py (1)
src/aiperf/common/tokenizer.py (1)
encode(81-96)
tests/unit/dataset/test_dataset_manager_inputs_json.py (1)
tests/unit/dataset/conftest.py (1)
capture_file_writes(64-96)
tests/unit/transports/test_aiohttp_client.py (2)
tests/unit/transports/conftest.py (2)
setup_mock_session(213-250)aiohttp_client(106-110)src/aiperf/transports/aiohttp_client.py (1)
post_request(134-146)
tests/unit/common/models/test_auto_routed_messages.py (4)
src/aiperf/common/messages/base_messages.py (1)
Message(15-60)src/aiperf/common/models/auto_routed_model.py (1)
from_json(87-110)src/aiperf/common/tokenizer.py (2)
encode(81-96)decode(98-113)src/aiperf/metrics/metric_dicts.py (1)
data(191-193)
⏰ 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). (11)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13)
- GitHub Check: integration-tests (ubuntu-latest, 3.13)
- GitHub Check: integration-tests (ubuntu-latest, 3.11)
- GitHub Check: build (macos-latest, 3.10)
- GitHub Check: build (macos-latest, 3.12)
- GitHub Check: integration-tests (ubuntu-latest, 3.12)
- GitHub Check: integration-tests (ubuntu-latest, 3.10)
- GitHub Check: build (macos-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.10)
🔇 Additional comments (23)
src/aiperf/common/config/user_config.py (2)
8-8: LGTM! Import updated correctly for msgspec.The import has been appropriately updated to use the msgspec module instead of orjson-specific imports.
144-144: LGTM! Exception handling updated correctly.The exception handling has been properly updated to catch
msgspec.DecodeErrorinstead of the previous orjson-specific exception, while preserving the KeyError handling.tests/aiperf_mock_server/config.py (2)
9-9: LGTM! Import added for msgspec.The msgspec import has been correctly added to support the updated serialization logic.
156-156: LGTM! Serialization correctly updated to msgspec.The serialization has been properly updated to use
msgspec.json.encode(), which returns bytes, followed by.decode("utf-8")to convert to a string suitable for environment variables.tests/integration/test_random_generator_canary.py (2)
13-13: LGTM! Import added for msgspec.The msgspec import has been correctly added to support the updated serialization logic in the test.
118-119: LGTM! File I/O correctly updated for msgspec.The changes correctly update the file I/O to use binary mode ("wb") with
msgspec.json.encode()andmsgspec.json.format()for pretty-printed JSON output. This aligns with msgspec's binary-first API design.tests/unit/common/models/test_sequence_distribution.py (1)
448-449: LGTM! Test naming updated to reflect msgspec migration.The test method name and docstring have been appropriately updated to reference msgspec.DecodeError instead of orjson.JSONDecodeError, while the test logic remains unchanged.
tests/integration/utils.py (3)
9-9: LGTM! Import updated to msgspec.The import has been correctly updated to use msgspec instead of json and orjson.
36-36: LGTM! Encoding updated to msgspec.The encoding has been properly updated to use
msgspec.json.encode(), which returns bytes, followed by.decode("utf-8")to convert to a string for writing to the file.
64-64: LGTM! Decoding updated to msgspec.The decoding has been correctly updated to use
msgspec.json.decode(), which can directly handle bytes from subprocess output.tests/unit/common/messages/test_messages.py (1)
3-3: msgspec-based JSON assertions are structurally sound and aligned with runtime behaviorSwitching these tests to use
msgspec.json.decodeon bothmodel_dump_json(...)output,to_json_bytes(), and__str__()output keeps assertions focused on the decoded dicts rather than raw string formatting. Given that production code now uses msgspec for JSON bytes, these changes look correct and consistent across the suite.Please confirm that the project is pinned to a msgspec version where
msgspec.json.decodesupports bothstrandbytesinputs, to ensure these tests remain valid across upgrades.Also applies to: 32-35, 52-55, 83-84, 100-102, 137-140, 142-144, 169-170, 231-233, 242-243, 255-256, 347-348
tests/integration/test_gpu_telemetry.py (1)
7-7: Telemetry JSONL decoding via msgspec is consistent with the new JSON backendDecoding each telemetry JSONL line with
msgspec.json.decodeand then validating viaTelemetryRecord.model_validate(...)keeps the test behavior equivalent while aligning with the project‑wide switch to msgspec. The type flow (str → dict → TelemetryRecord) looks correct.Please ensure the msgspec version in your environment matches the one assumed in the project configuration so that
msgspec.json.decodebehavior (on str input) remains stable for these integration tests.Also applies to: 99-102
tests/unit/transports/test_aiohttp_transport.py (1)
221-239: Test rename accurately reflects the msgspec-based serialization in the transportRenaming the test and docstring from “...with_orjson” to “...with_msgspec” brings the test in line with the underlying transport implementation while still validating the same behavior (that a JSON string is produced and contains expected fields). No additional assertions seem necessary here.
If you want stronger coupling to the backend choice, consider asserting that the serialized output matches
msgspec.json.encode(payload).decode("utf-8")exactly; otherwise, this looser behavior-based check is fine.tests/unit/server/test_config.py (1)
109-112: Updated list serialization expectation matches compact JSON outputChanging the expectation to
'["a","b"]'reflects the more compact JSON formatting produced by the new serializer. This is appropriate as long as_serialize_env_valueis intended to emit minified JSON for complex env values and tests are expected to track that exact format.Please confirm that
_serialize_env_valuenow uses msgspec (or another minifying JSON backend) consistently so this expectation doesn’t drift again with future formatter changes.tests/unit/dataset/test_dataset_manager_inputs_json.py (1)
11-11: Inputs JSON tests now decode with msgspec in a way that mirrors production behaviorDecoding the generated
inputs.jsoncontent (both via the async capture fixture and from disk) withmsgspec.json.decodebefore asserting structure or validating withInputsFile.model_validate(...)keeps these tests aligned with the new JSON serialization path. The type flow and assertions all look correct.Please ensure DatasetManager’s
_generate_inputs_json_fileis also using msgspec (or a compatible JSON backend) so that these tests always exercise the same code path as production writes.Also applies to: 55-56, 83-84, 100-102, 113-117, 127-128, 147-148
src/aiperf/common/models/record_models.py (1)
10-10: JSON decode error handling correctly migrated to msgspecImporting
msgspecand catchingmsgspec.DecodeErrorin bothTextResponse.get_jsonandSSEMessage.get_jsonis consistent with the updatedload_json_strimplementation and preserves the previous “invalid JSON → return None” behavior without changing the public API.Also applies to: 247-255, 342-352
tests/unit/common/mixins/test_buffered_jsonl_writer_mixin.py (1)
8-8: Test JSON decoding aligned with msgspec writer implementationUsing
msgspec.json.decode(line)in the integrity test matches the production writer’s msgspec-based encoding and keeps the assertion ("id" in ...) valid on the decoded dict.Also applies to: 69-69
tests/unit/post_processors/test_raw_record_writer_processor.py (1)
4-4: Raw record tests consistently migrated to msgspec decodingReplacing prior JSON deserialization with
msgspec.json.decode(...)across these tests cleanly aligns them with the new serialization pipeline, and feeding the resulting dicts intoRawRecordInfo.model_validatekeeps type and structure validation intact.Also applies to: 106-107, 133-135, 166-167, 195-197, 552-553, 634-635
tests/unit/exporters/test_timeslice_metrics_json_exporter.py (1)
10-10: Timeslice metrics exporter tests now validate JSON via msgspec consistentlySwitching all the JSON parsing in this module to
msgspec.json.decode(...)keeps the tests aligned with the exporter’s serialization backend, while the subsequent dict‑based assertions (and the separateTimesliceCollectionExportData.model_validate_jsoncheck) still robustly validate structure and content.Also applies to: 249-253, 295-299, 352-357, 406-413, 460-471, 551-553, 633-635
tests/unit/post_processors/test_record_export_results_processor.py (1)
8-8: msgspec-based JSON decoding in tests looks correctDecoding JSONL lines via
msgspec.json.decode(...)and then validating withMetricRecordInfo.model_validate(...)is a sound replacement for the previous orjson/json usage; the inputs arestrfromread_text(), which msgspec handles natively.Also applies to: 231-232, 340-342, 376-381, 408-410
tests/unit/post_processors/test_telemetry_export_results_processor.py (1)
8-8: msgspec.json.decode usage integrates cleanly with telemetry testsDecoding each JSONL line with
msgspec.json.decode(...)and then either feeding the result intoTelemetryRecord.model_validate(...)or inspecting the dict directly is a correct replacement for the prior orjson/json-based decoding, given that lines are read asstr. The tests that assert omission ofNonefields remain consistent with the writer’sexclude_nonebehavior.Also applies to: 231-232, 261-262, 408-409, 443-444, 508-509, 533-535, 563-564, 609-610, 645-646, 668-669, 698-699, 976-977, 1092-1093
tests/unit/exporters/test_metrics_json_exporter.py (1)
8-8: JSON decoding via msgspec in exporter tests is appropriateUsing
msgspec.json.decode(...)on the exporter’s JSON payload (both from_generate_content()and from files) to obtain dicts for assertions is a correct adaptation to the msgspec backend and interoperates cleanly with the existingJsonExportData.model_validate_jsonusage.Also applies to: 216-217, 247-248, 315-317, 359-361, 385-387, 469-470, 525-526, 562-563, 621-622, 704-705, 768-769
src/aiperf/common/mixins/buffered_jsonl_writer_mixin.py (1)
10-11: msgspec-based JSONL serialization is consistent; confirm this is the new standard backendThe updated writer flow—
- binary open (
"wb"),record.model_dump(exclude_none=True, mode="json"),msgspec.json.encode(...)to bytes buffered and flushed in batches—fits the existing buffered JSONL design and aligns with tests that assert
Nonefields are omitted from the serialized output. Functionality and concurrency behavior look intact.One meta point: project coding guidelines still say “Always use orjson for JSON operations”. If msgspec is now intended to be the canonical JSON backend (as this mixin and associated tests indicate), those guidelines should be updated so this implementation is explicitly in-policy.
As per coding guidelines, the preferred JSON library should be kept in sync with implementations like this mixin.
Also applies to: 60-62, 66-74, 79-83
| "ffmpeg-python~=0.2.0", | ||
| "jinja2~=3.1.5", # NOTE: Versions prior to 3.1.5 have vuln exploits | ||
| "jmespath~=1.0.1", | ||
| "msgspec~=0.20.0", |
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.
🧩 Analysis chain
Verify msgspec version compatibility.
The addition of msgspec~=0.20.0 looks good. Ensure this version is compatible with Python 3.10+ and provides the required JSON encoding/decoding APIs used throughout the codebase.
🌐 Web query:
What is the latest stable version of msgspec and does version 0.20.0 support Python 3.10+?
💡 Result:
-
The official msgspec latest stable release is 0.19.0 (published Dec 27, 2024). [1]
-
msgspec 0.19.0 provides wheels for CPython 3.10 (and newer CPython versions listed on the release files), so the official msgspec release line supports Python 3.10+. [1]
-
There is a community fork named msgspec-x which has a 0.20.0 release (Jun 21, 2025). That fork’s 0.20.0 lists support for CPython 3.10+ in its PyPI release metadata. If you meant the official project, 0.20.0 is not an official msgspec release; 0.19.0 is the latest official stable. [2]
Sources:
[1] msgspec on PyPI — release files and metadata.
[2] msgspec-x on PyPI (version 0.20.0).
I need to check the codebase to determine which msgspec package is intended (official or the fork).
🏁 Script executed:
#!/bin/bash
# Search for msgspec imports and usage in codebase
rg -n "msgspec|from msgspec|import msgspec" --type py -C 3Length of output: 77213
🏁 Script executed:
#!/bin/bash
# Check pyproject.toml for any comments or context about msgspec
cat pyproject.toml | head -50Length of output: 1241
🏁 Script executed:
#!/bin/bash
# Check if msgspec-x is mentioned anywhere
rg -n "msgspec-x|msgspec_x" --type py --type toml --type txtLength of output: 0
Fix invalid msgspec version constraint that will cause installation failures.
Official msgspec's latest stable version is 0.19.0, not 0.20.0. The current constraint msgspec~=0.20.0 cannot be satisfied by the official package, causing pip install to fail. Version 0.19.0 supports Python 3.10+ as required by your project. Either downgrade to msgspec~=0.19.0 or explicitly specify the msgspec-x fork if version 0.20.0 is intentional.
🤖 Prompt for AI Agents
In pyproject.toml around line 33, the dependency constraint "msgspec~=0.20.0" is
invalid (official latest is 0.19.0) and will break installation; update the
entry to "msgspec~=0.19.0" to match the published package, or if you
intentionally require the 0.20.0 fork, replace the dependency with the correct
fork package name and version (e.g., specify the msgspec-x fork and its
compatible version) so pip can resolve and install it successfully.
| from typing import Any | ||
|
|
||
| import orjson | ||
| import msgspec |
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.
🧩 Analysis chain
Fix bytes/str mixing in load_json_str error logging and update JSON backend guidance
When json_str is bytes and decoding fails, the except block does:
snippet = json_str[:200] + ("..." if len(json_str) > 200 else "")For bytes input this becomes bytes + str, which raises TypeError inside the except handler and masks the original msgspec.DecodeError. That can surprise callers that now expect msgspec.DecodeError on parse failure.
You can keep the msgspec backend but make the snippet construction bytes‑safe like this:
def load_json_str(
json_str: str | bytes, func: Callable = lambda x: x
) -> dict[str, Any]:
@@
- try:
- # Note: msgspec is strict on UTF-8 conformance and provides fast JSON parsing
- return func(msgspec.json.decode(json_str))
- except msgspec.DecodeError as e:
- snippet = json_str[:200] + ("..." if len(json_str) > 200 else "")
- _logger.exception(f"Failed to parse JSON string: '{snippet}' - {e!r}")
- raise
+ try:
+ # Note: msgspec is strict on UTF-8 conformance and provides fast JSON parsing
+ return func(msgspec.json.decode(json_str))
+ except msgspec.DecodeError as e:
+ if isinstance(json_str, bytes):
+ snippet = json_str[:200].decode("utf-8", errors="replace")
+ else:
+ snippet = json_str[:200]
+ if len(json_str) > 200:
+ snippet += "..."
+ _logger.exception(f"Failed to parse JSON string: '{snippet}' - {e!r}")
+ raiseSeparately, the current CLAUDE/guidelines still say “Always use orjson for JSON operations”, but this helper (and the PR as a whole) intentionally standardizes on msgspec. It would be good to update those docs to avoid future confusion. Based on learnings, this keeps the documented guidance in sync with the new implementation.
Also applies to: 79-99
🏁 Script executed:
#!/bin/bash
# Inspect load_json_str implementation to verify bytes/str mixing issue
cat src/aiperf/common/utils.py | sed -n '1,120p'Length of output: 4041
🏁 Script executed:
#!/bin/bash
# Check CLAUDE.md for orjson guidance that may be outdated
fd -e md --type f | head -20 | xargs grep -l "orjson" 2>/dev/null || echo "No orjson references found in md files"Length of output: 22
🏁 Script executed:
#!/bin/bash
# Verify msgspec API: check if msgspec.json.decode and msgspec.DecodeError are valid
python3 << 'EOF'
import msgspec
# Check if msgspec.json.decode exists
print("msgspec.json.decode exists:", hasattr(msgspec.json, 'decode'))
# Check if msgspec.DecodeError exists
print("msgspec.DecodeError exists:", hasattr(msgspec, 'DecodeError'))
# Test bytes + str behavior
test_bytes = b"test"[:3]
print(f"Type of bytes[:3]: {type(test_bytes)}")
try:
result = test_bytes + "..."
except TypeError as e:
print(f"bytes + str raises TypeError: {e}")
EOFLength of output: 119
[/run_scripts]
[/web_search]
🏁 Script executed:
#!/bin/bash
# Check ATTRIBUTIONS-Python.md and other docs for orjson and messaging guidance
grep -r "orjson\|msgspec" --include="*.md" .Length of output: 375
🏁 Script executed:
#!/bin/bash
# Look for coding guidelines or CLAUDE files mentioning JSON
fd -e md --type f -name "*CLAUDE*" -o -name "*guideline*" -o -name "*coding*" | xargs cat 2>/dev/null | head -100Length of output: 230
🌐 Web query:
msgspec.json.decode and msgspec.DecodeError Python API documentation
💡 Result:
Here’s the short API info and examples for msgspec.json.decode and the DecodeError/ValidationError behavior.
-
msgspec.json.decode(buf, *, type: Optional[type] = None, strict: bool = True, dec_hook: Optional[callable] = None) -> Any
- buf: bytes-like or str containing the JSON to decode.
- type: optional Python type (annotation form) to decode/validate into; if omitted decodes to default JSON->Python types.
- strict: when False, allows wider coercions from strings to non-string types.
- dec_hook: optional callback dec_hook(type, obj) -> Any for custom-type construction.
- Returns the deserialized Python object. [1][2]
-
Errors:
- msgspec.DecodeError / msgspec.ValidationError (raised on invalid JSON or when decoded data doesn’t match the requested type). The library raises a DecodeError (or ValidationError for type-validation failures) with a message describing the problem and a JSON path (e.g. "$.groups[0]"). Example: decoding b'{"name":"bob","groups":[123]}' as a type expecting str in groups yields an error like "Expected
str, gotint- at$.groups[0]." [2][3]
- msgspec.DecodeError / msgspec.ValidationError (raised on invalid JSON or when decoded data doesn’t match the requested type). The library raises a DecodeError (or ValidationError for type-validation failures) with a message describing the problem and a JSON path (e.g. "$.groups[0]"). Example: decoding b'{"name":"bob","groups":[123]}' as a type expecting str in groups yields an error like "Expected
-
Example
- encode: msg = msgspec.json.encode(obj)
- decode without type: msgspec.json.decode(msg) -> dict/list/...
- decode with type: msgspec.json.decode(msg, type=User) -> User (or raises ValidationError). [2][3]
Sources:
- msgspec API docs (json.decode parameters & description). [1]
- msgspec usage and examples (encoding/decoding, typed decoding, error examples). [2][3]
References
[1] msgspec API docs — JSON (decode).
[2] msgspec Usage / Examples (encode/decode, typed decoding).
[3] msgspec GitHub / README (examples showing DecodeError/ValidationError messages).
Fix bytes/str mixing in load_json_str error logging
When json_str is bytes and decoding fails, the except block does:
snippet = json_str[:200] + ("..." if len(json_str) > 200 else "")For bytes input this becomes bytes + str, which raises TypeError inside the except handler and masks the original msgspec.DecodeError. That can surprise callers that expect msgspec.DecodeError on parse failure.
Fix by normalizing snippet to str:
def load_json_str(
json_str: str | bytes, func: Callable = lambda x: x
) -> dict[str, Any]:
@@
- try:
- # Note: msgspec is strict on UTF-8 conformance and provides fast JSON parsing
- return func(msgspec.json.decode(json_str))
- except msgspec.DecodeError as e:
- snippet = json_str[:200] + ("..." if len(json_str) > 200 else "")
- _logger.exception(f"Failed to parse JSON string: '{snippet}' - {e!r}")
- raise
+ try:
+ # Note: msgspec is strict on UTF-8 conformance and provides fast JSON parsing
+ return func(msgspec.json.decode(json_str))
+ except msgspec.DecodeError as e:
+ if isinstance(json_str, bytes):
+ snippet = json_str[:200].decode("utf-8", errors="replace")
+ else:
+ snippet = json_str[:200]
+ if len(json_str) > 200:
+ snippet += "..."
+ _logger.exception(f"Failed to parse JSON string: '{snippet}' - {e!r}")
+ raise
Important
Converting to draft as still running benchmarks to compare, and the jury is still out
Replaces all usage of
jsonandorjsonwithmsgspec.jsonfor better performance. See: https://github.com/jcrist/msgspecSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.