fix(profiler): propagate spec.sla.{ttft,itl} into PlannerConfig - #9537
Merged
hhzhang16 merged 12 commits intoMay 20, 2026
Conversation
'_build_planner_config()' in 'components/src/dynamo/profiler/utils/dgd_generation.py' builds the 'PlannerConfig'. 'PlannerConfig' silently drops 'dgdr.sla.ttft' and 'dgdr.sla.itl' fields the user already provided as the SLA targets for the deployment, when deploying DGDR. The result: the post-deployment Planner enforces 'SLAPlannerDefaults.ttft_ms = 500.0' and'SLAPlannerDefaults.itl_ms = 50.0' (dynamo/components/src/dynamo/planner/config/defaults.py, class SLAPlannerDefualts) regardless of what the user wrote in 'spec.sla'. This violates the implicit contract that 'spec.sla' is the canonical SLA target for the whole DGDR. Signed-off-by: jooe0824 <jooe0824@gmail.com>
Contributor
|
👋 Hi jooe0824! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
external-contribution
Contributor
WalkthroughThis PR adds SLA timing propagation logic to the planner configuration builder. When SLA targets exist (ttft and itl) but e2eLatency is unset, the builder now maps these values into the generated PlannerConfig, unless the fields were explicitly overridden. ChangesSLA Timing Propagation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/src/dynamo/profiler/utils/dgd_generation.py (1)
500-506: ⚡ Quick winConvert the triple-quoted string to regular comments.
The triple-quoted string at lines 500–506 is being used as a block comment, which violates PEP 8 conventions. PEP 8 reserves triple-quoted strings for docstrings (module/class/function documentation), and requires inline comments to use
#.As per coding guidelines, this file must follow PEP 8.
♻️ Suggested fix
- """ - Propagate SLA targets from spec.sla so the post-deployment planner - enforces the same SLA used at sweep time. Without this, the planner silently uses SLAPlannerDefaults - ttft_ms=500 / itl_ms=50 - - Explicit user overrides on features.planner.{ttft_ms, itl_ms} take precedence - detected via model_fields_set - """ + # Propagate SLA targets from spec.sla so the post-deployment planner + # enforces the same SLA used at sweep time. Without this, the planner + # silently uses SLAPlannerDefaults ttft_ms=500 / itl_ms=50. + # + # Explicit user overrides on features.planner.{ttft_ms, itl_ms} take + # precedence - detected via model_fields_set.🤖 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 `@components/src/dynamo/profiler/utils/dgd_generation.py` around lines 500 - 506, The triple-quoted block starting with "Propagate SLA targets from spec.sla..." should be converted to PEP8-compliant inline comments: replace the """"..."""" block with one or more lines prefixed by #, keeping the same wording and references to spec.sla, SLAPlannerDefaults, features.planner.{ttft_ms, itl_ms}, and model_fields_set so the intent is preserved; ensure the comment is placed immediately where the original block was (in dgd_generation.py) and retain the note about explicit overrides taking precedence.
🤖 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 `@components/src/dynamo/profiler/utils/dgd_generation.py`:
- Around line 500-506: The triple-quoted block starting with "Propagate SLA
targets from spec.sla..." should be converted to PEP8-compliant inline comments:
replace the """"..."""" block with one or more lines prefixed by #, keeping the
same wording and references to spec.sla, SLAPlannerDefaults,
features.planner.{ttft_ms, itl_ms}, and model_fields_set so the intent is
preserved; ensure the comment is placed immediately where the original block was
(in dgd_generation.py) and retain the note about explicit overrides taking
precedence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76612337-03e8-4ee8-9f27-2d6520a60311
📒 Files selected for processing (1)
components/src/dynamo/profiler/utils/dgd_generation.py
dynamo-ops
approved these changes
May 14, 2026
hhzhang16
reviewed
May 18, 2026
…add unit tests Addresses review on ai-dynamo#9537. valid_dgdr_spec() injects a defaulted SLASpec(ttft=2000, itl=30) when spec.sla is omitted, so the previous check silently overwrote SLAPlannerDefaults (500/50) for deployments without spec.sla. The propagation now requires both 'ttft' and 'itl' in sla.model_fields_set and is skipped entirely in e2eLatency mode. Explicit features.planner overrides retain precedence. Adds test_dgd_generation_planner_sla.py covering: - Backwards- compatibility preservation when spec.sla is omitted or set as empty SLASpec() - forward propagation of explicit sla.ttft/itl - precedence of features.planner.{ttft_ms,itl_ms} overrides - e2eLatency mode skipping ttft/itl propagation Signed-off-by: jooe0824 <jooe0824@sk.com>
Contributor
|
/ok to test d2ff3b6 |
hhzhang16
reviewed
May 19, 2026
Previously both ttft and itl had to be in model_fiels_set for SLA values to flow into the planner config. Change the gate to OR so a partial spec still propagates, Adds unit tests covering ttft-only and itl-only paths Signed-off-by: jooe0824 <jooe0824@sk.com>
…/jooe0824/dynamo into jooe0824/planner-sla-passthrough
hhzhang16
approved these changes
May 20, 2026
Contributor
|
/ok to test 17d1e96 |
Contributor
Author
|
Hi @hhzhang16, thank you again for reviewing my earlier profiler PR! 🙏
It's a small change with unit tests included. Whenever you have time, I'd really appreciate it if you could take a look. Thank you! |
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.
Overview:
_build_planner_config()incomponents/src/dynamo/profiler/utils/dgd_generation.pybuilds the 'PlannerConfig'. 'PlannerConfig' silently dropsdgdr.sla.ttftanddgdr.sla.itlfields the user already provided as the SLA targets for the deployment, when deploying DGDR. The result: the post-deployment Planner enforcesSLAPlannerDefaults.ttft_ms = 500.0andSLAPlannerDefaults.itl_ms = 50.0(components/src/dynamo/planner/config/defaults.py,class SLAPlannerDefaults) regardless of what the user wrote inspec.sla. This violates the implicit contract thatspec.slais the canonical SLA target for the whole DGDR.SLASpec(DGDR)spec.slaPlannerConfig(current bug)PlannerConfig(with fix)ttftSLAPlannerDefaults)itlSLAPlannerDefaults)Details:
At the end of _build_planner_config() (before return planner_cfg), propagate
dgdr.sla.{ttft,itl}into planner_cfg.{ttft_ms,itl_ms} — unless the user explicitly set those fields on features.planner.spec.sla.e2eLatencyis left as-is — PlannerConfig has no e2e_latency field, so any e2e enforcement requires explicit conversion to ttft/itl, which depends on workload characteristics the profiler doesn't have a principled rule for.Where should the reviewer start?
components/src/dynamo/profiler/utils/dgd_generation.py:479— the _build_planner_config function (lines 479–525 after this patch). The change is a 24-line append (including an 8-line comment block explaining themodel_fields_setidiom and the e2eLatency skip rationale) at the end of the function beforereturn planner_cfg.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to issue #8469 ("[BUG][GROUP]: fix DGDR profiling and deployment errors on existing models and backends") — natural sub-issue under that bucket.
Tests: A test file is ready locally and can be added to this PR or submitted as a follow-up — whichever the reviewer prefers.
Summary by CodeRabbit