fix: correct planner test example after tokenizer fix #2674
Conversation
WalkthroughUpdated import path for create_sla_planner_parser in planner code and its test. Adjusted tests/planner/README.md to new interpolator outputs, workload expectations, dataset naming, and example commands. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/planner/test/planner_sla_dryrun.py (1)
24-47: Fail fast if the dataset path is invalid.Small UX improvement: validate
--datasetbefore running so users get an immediate, actionable error.Apply this diff:
@@ -import logging +import logging +from pathlib import Path @@ parser = create_sla_planner_parser() parser.add_argument( "--dataset", type=str, required=True, help="Path to the jsonl dataset file" ) @@ ) args = parser.parse_args() + dataset_path = Path(args.dataset) + if not dataset_path.is_file(): + parser.error(f"Dataset file not found: {dataset_path}") + planner = Planner(None, args, dryrun=True) planner.dryrun_run()tests/planner/README.md (2)
70-72: Tighten wording and unit pluralization.Minor grammar/clarity nits.
Apply this diff:
-From previous interpolator testing, ISL 3000 and OSL 300 can handle ~15 request/s/gpu for both prefill and decode. -To test planner's performance for different request rates, we can generate a load dataset with request rate varying between 12 to 36 request/s. -For TP1 H200 engine, planner should scale between 1P1D and 3P3D. +Based on previous interpolator testing, ISL 3000 and OSL 300 can handle ~15 requests/s/GPU for both prefill and decode. +To test the planner's performance across different request rates, generate a load dataset with a request rate varying from 12 to 36 requests/s. +For a TP1 H200 engine, the planner should scale between 1P1D and 3P3D.
107-107: Hyphenate “dry-run” and add article.Apply this diff:
-For example, to dry run SLA planner for the previous FP8 8B on H200 using the generated `rr-12-36_i3000o300.jsonl` dataset, +For example, to dry-run the SLA planner for the previous FP8 8B on H200 using the generated `rr-12-36_i3000o300.jsonl` dataset,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
tests/planner/figures/dryrun_plot.pngis excluded by!**/*.png
📒 Files selected for processing (3)
components/planner/src/dynamo/planner/planner_sla.py(1 hunks)components/planner/test/planner_sla_dryrun.py(1 hunks)tests/planner/README.md(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/planner/src/dynamo/planner/planner_sla.py (1)
components/planner/src/dynamo/planner/utils/planner_argparse.py (1)
create_sla_planner_parser(21-122)
components/planner/test/planner_sla_dryrun.py (1)
components/planner/src/dynamo/planner/utils/planner_argparse.py (1)
create_sla_planner_parser(21-122)
🪛 LanguageTool
tests/planner/README.md
[grammar] ~70-~70: There might be a mistake here.
Context: ...quest/s/gpu for both prefill and decode. To test planner's performance for differ...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...t/s/gpu for both prefill and decode. To test planner's performance for different req...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...rate varying between 12 to 36 request/s. For TP1 H200 engine, planner should scal...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ... 12 to 36 request/s. For TP1 H200 engine, planner should scale between 1P1D and 3...
(QB_NEW_EN)
[grammar] ~107-~107: There might be a mistake here.
Context: ...o_output_plot> ``` For example, to dry run SLA planner for the previous FP8 8B on ...
(QB_NEW_EN)
⏰ 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). (5)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: pre-merge-rust (.)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (5)
components/planner/src/dynamo/planner/planner_sla.py (3)
22-22: Import path rename looks good and avoids stdlib shadowing.Switching to
planner_argparseeliminates accidental conflicts with Python’sargparse. No functional impact beyond the import path.
54-57: Sanity check: main invocation vs decorator-injected runtime.
init_planneris declared asasync def init_planner(runtime: DistributedRuntime, args)but__main__callsasyncio.run(init_planner(args)). If@dynamo_workerrewrites the callable signature for CLI use, this is fine; otherwise this will raise a missing-argument error at runtime.If direct CLI execution is intended and the decorator does not alter the signature, consider providing a thin
async def _main(args)that constructs aDistributedRuntime(or obtains one from your runtime layer) and then callsawait init_planner(runtime, args). Let me know and I can draft the exact glue once I see howDistributedRuntimeis instantiated in this codebase.
22-22: No lingeringdynamo.planner.utils.argparseimports detected.
- Verified that all explicit imports of the old module path have been removed.
- Confirmed there are no string literals or dynamic import calls referencing
dynamo.planner.utils.argparse.- The
components/planner/src/dynamo/planner/planner_sla.pyimport on line 22 has been correctly updated to usedynamo.planner.utils.planner_argparse.No further action is required.
components/planner/test/planner_sla_dryrun.py (2)
18-18: Import path update is correct.Aligned with the module rename to
planner_argparse. No other changes required here.
18-18: No remaining imports of the old module path
I’ve run a repository-wide search fordynamo.planner.utils.argparseand found zero occurrences—there are no lingering references.
Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Signed-off-by: Jason Zhou <jasonzho@jasonzho-mlt.client.nvidia.com>
Signed-off-by: Krishnan Prashanth <kprashanth@nvidia.com>
Signed-off-by: nnshah1 <neelays@nvidia.com>
argparse.pyto distinguish from argparse packageSummary by CodeRabbit
Documentation
Refactor
Tests