fix: Fix/export outputs json cli flag#1138
Open
dferguson992 wants to merge 6 commits into
Open
Conversation
Signed-off-by: Dan Ferguson <frgud@amazon.com>
Restore CLI flag binding for export_outputs_json, which controls whether outputs.json (the raw generated-response-text dump) is written after a benchmark run. The setting was only reachable via the nested YAML config field benchmark.artifacts.export_outputs_json. This adds a top-level --export-outputs-json flag to the profile command. Closes ai-dynamo#1133 Signed-off-by: Dan Ferguson <frgud@amazon.com>
dferguson992
requested review from
FrankD412,
ajcasagrande,
debermudez,
ganeshku1,
ilana-n,
lkomali and
matthewkotila
as code owners
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@f580d20828005846f5cffc3d7488b05d0a9bbf55Recommended 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@f580d20828005846f5cffc3d7488b05d0a9bbf55Last updated for commit: |
dferguson992
changed the title
Contributor
WalkthroughAdds the ChangesExport Outputs JSON Flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ajcasagrande
reviewed
Jul 9, 2026
ajcasagrande
left a comment
ajcasagrande
left a comment
Contributor
There was a problem hiding this comment.
Thanks for adding the CLI wiring for outputs.json; the basic happy path works — I verified --export-outputs-json with the real aiperf profile CLI against the in-repo mock server and it produced the expected generated-output records, and the related unit tests pass.
I found three issues to address before merging, in this order:
- Fix the
--profile-export-prefix outputscollision first; it corruptsoutputs.jsoninto invalid JSON. - Clean or scope
output_fragmentsso retries after failed runs cannot mix stale responses into the new artifact. - Decide whether
--export-outputs-jsonshould force per-record JSONL when YAML hadartifacts.records: false; if yes, document it explicitly, otherwise avoid emittingrecordsunless--export-levelwas set.
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Dan Ferguson <frgud@amazon.com>
![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: 2
🧹 Nitpick comments (1)
tests/unit/config/test_outputs_json_prefix_collision.py (1)
24-39: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd normalized-path collision cases.
The parameter list covers suffix stripping but not prefixes such as
./outputsorsub/../outputs; these currently bypass the new guard.Proposed test cases
param("outputs_timeslices.json", id="with-timeslices-suffix"), + param("./outputs", id="dot-path"), + param("sub/../outputs", id="normalized-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 `@tests/unit/config/test_outputs_json_prefix_collision.py` around lines 24 - 39, Add parameterized cases for normalized equivalent prefixes, including "./outputs" and "sub/../outputs", to test_rejects_colliding_prefix in the existing prefix list. Keep the expected ValidationError assertion unchanged so ArtifactsConfig rejects these paths when export_outputs_json is enabled.
🤖 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/config/artifacts.py`:
- Around line 224-235: Update _check_outputs_json_collision to compare
normalized filesystem paths rather than raw strings: resolve
self.profile_export_json_file and self.outputs_json_file and reject them when
equal, preserving the existing base/optional-prefix handling.
In `@tests/unit/config/test_outputs_json_prefix_collision.py`:
- Around line 35-39: Rename the affected tests to follow
test_<function>_<scenario>_<expected>, including the tested function, scenario,
and outcome; for example, rename the ArtifactsConfig collision test to
test_artifacts_config_rejects_colliding_prefix and apply equivalent descriptive
names to the tests covering convert_cli_to_aiperf and the other indicated cases.
---
Nitpick comments:
In `@tests/unit/config/test_outputs_json_prefix_collision.py`:
- Around line 24-39: Add parameterized cases for normalized equivalent prefixes,
including "./outputs" and "sub/../outputs", to test_rejects_colliding_prefix in
the existing prefix list. Keep the expected ValidationError assertion unchanged
so ArtifactsConfig rejects these paths when export_outputs_json is enabled.
🪄 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: 28ce16bc-41e2-4794-8856-d0c211bc253c
📒 Files selected for processing (5)
docs/cli-options.mdsrc/aiperf/config/artifacts.pysrc/aiperf/config/flags/_converter_runtime.pysrc/aiperf/config/resolution/resolvers.pytests/unit/config/test_outputs_json_prefix_collision.py
Signed-off-by: Dan Ferguson <frgud@amazon.com>
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/config/test_resolvers.py (1)
272-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to the new test methods.
Annotate the fixture parameters and return types, for example with the project’s config type,
Path, and-> None.As per coding guidelines, add type hints to all Python function parameters and return values.
Also applies to: 289-307
🤖 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/config/test_resolvers.py` around lines 272 - 287, The new test methods, including test_resolve_purges_stale_output_fragments and the methods around lines 289-307, lack type annotations. Add types for every fixture parameter using the project’s config type and Path where applicable, and annotate each test method with -> None.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.
Nitpick comments:
In `@tests/unit/config/test_resolvers.py`:
- Around line 272-287: The new test methods, including
test_resolve_purges_stale_output_fragments and the methods around lines 289-307,
lack type annotations. Add types for every fixture parameter using the project’s
config type and Path where applicable, and annotate each test method with ->
None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8eb70bfa-3e28-405f-a407-cebd90d62f57
📒 Files selected for processing (3)
src/aiperf/config/artifacts.pytests/unit/config/test_outputs_json_prefix_collision.pytests/unit/config/test_resolvers.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/aiperf/config/artifacts.py
- tests/unit/config/test_outputs_json_prefix_collision.py
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
3 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.
Attempts to fix Issue #1133
Summary by CodeRabbit
New Features
--export-outputs-jsonto export each request’s generated response text to anoutputs.jsonfile in the artifacts directory (for both profiling and service workflows).Bug Fixes
--profile-export-prefixand the generated outputs target when export is enabled.export_levelwas explicitly set.Documentation
Tests