Skip to content

fix: json strings should remain intact through profiler arg processing#3680

Merged
hhzhang16 merged 5 commits into
mainfrom
bug--5587094-fix-main
Oct 17, 2025
Merged

fix: json strings should remain intact through profiler arg processing#3680
hhzhang16 merged 5 commits into
mainfrom
bug--5587094-fix-main

Conversation

@hhzhang16
Copy link
Copy Markdown
Contributor

@hhzhang16 hhzhang16 commented Oct 16, 2025

Overview:

Details:

Where should the reviewer start?

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • closes GitHub issue: #xxx

Summary by CodeRabbit

  • Bug Fixes
    • Improved argument parsing in benchmark profiler configurations to correctly handle JSON-formatted values and single tokens. Arguments resembling JSON structures or single tokens are now properly preserved during processing.

Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
@hhzhang16 hhzhang16 requested a review from a team as a code owner October 16, 2025 20:37
@hhzhang16 hhzhang16 requested a review from a team October 16, 2025 20:37
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Oct 16, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Oct 16, 2025

Walkthrough

The break_arguments function in the profiler utilities now conditionally splits arguments based on JSON detection. Arguments appearing to be JSON structures or single tokens are preserved as-is, while multi-token non-JSON arguments continue to be split on whitespace.

Changes

Cohort / File(s) Summary
Argument parsing refinement
benchmarks/profiler/utils/config.py
Modified break_arguments to detect JSON-like tokens (starting with { or [) and single tokens, preserving them from whitespace-based splitting while maintaining previous split behavior for multi-token arguments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 With brackets spared and braces whole,
Our parser now preserves each soul,
No JSON torn by splitting's might—
Arguments stay shiny, clean, and bright!

Pre-merge checks

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description contains the correct section structure from the template but lacks any substantive content. All required sections—Overview, Details, Where should the reviewer start?, and Related Issues—remain unfilled with only placeholder comments and the generic issue reference "#xxx". This represents a largely incomplete description that does not provide reviewers with essential information about what was changed, why these changes were made, or which files require close review. Please provide substantive content for each section of the description. Add a clear overview explaining the problem being fixed, describe the specific changes made and their rationale in the Details section, identify the key files to review (particularly benchmarks/profiler/utils/config.py based on the changes), and reference the actual GitHub issue number that this pull request resolves instead of the placeholder "#xxx".
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: json strings should remain intact through profiler arg processing" directly reflects the main change described in the raw summary. The modification to the break_arguments function specifically prevents JSON-like strings (those starting with { or [) from being split, which is exactly what the title conveys. The title is concise, specific, and immediately communicates the primary fix without vague terminology or misleading information.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 333ee98 and f180081.

📒 Files selected for processing (1)
  • benchmarks/profiler/utils/config.py (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
benchmarks/profiler/utils/config.py (1)

128-138: The review's concerns about JSON string detection do not apply to actual codebase usage.

The heuristic in the modified code is correct and sufficient. Analysis shows:

  1. json.dumps() usage in codebase: Only serializes dictionaries (override_dict), which always produce strings starting with {, never ".

  2. JSON string handling: While json.dumps("string_value") would theoretically produce "..." starting with ", the codebase never does this. All override arguments are created via json.dumps(override_dict), not string serialization.

  3. Heuristic coverage: The check arg.strip().startswith(("{", "[")) correctly covers all actual JSON patterns in use:

    • --override-engine-args '{"key": "value"}' ✓ (dict → starts with {)
    • --override-engine-args '[...]' ✓ (list → starts with [)

The fix resolves the actual issue (preserving JSON objects/arrays during argument parsing) and works correctly for all real use cases.

Likely an incorrect or invalid review comment.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
@hhzhang16 hhzhang16 enabled auto-merge (squash) October 16, 2025 21:13
@hhzhang16
Copy link
Copy Markdown
Contributor Author

/ok to test de83a60

@hhzhang16 hhzhang16 merged commit 9206403 into main Oct 17, 2025
28 of 30 checks passed
@hhzhang16 hhzhang16 deleted the bug--5587094-fix-main branch October 17, 2025 00:13
hhzhang16 added a commit that referenced this pull request Oct 17, 2025
#3680)

Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
ziqifan617 pushed a commit that referenced this pull request Oct 20, 2025
yao531441 pushed a commit to yao531441/dynamo that referenced this pull request May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants