[llmd] Work on Azure#930
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR updates LLM-D testing infrastructure by introducing a new Azure H100 CI preset with related configuration toggles, adding HTTPRoute capture tasks to Ansible playbooks for improved debugging during service deployment, and disabling specific visualization reports while streamlining baseline flavor filtering in plotting logic. ChangesCI Preset and Feature Configuration
HTTPRoute Debug Artifact Capture
Visualization Report Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
projects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.py (1)
324-335: ⚡ Quick winRemove unused
filter_flavorsfunction or refactor to use it.The
filter_flavorsfunction is defined but never called withinBaselineComparisonsReport.do_plot. Meanwhile, the newly added flavor filtering at lines 348-349 uses a simpleif/continuepattern instead.The other report classes (
IntelligentRoutingComparisonsReportandPDComparisonsReport) define identicalfilter_flavorsfunctions and actually use them for filtering. For consistency and to avoid dead code, consider either:
- Remove this unused function (simpler if the inline skip is sufficient), or
- Refactor lines 348-349 to use
filter_flavorsfor consistency with other reports.Option 1: Remove the unused function
- def filter_flavors(setting_lists, flavor_filter): - """Filter flavors from setting_lists based on provided filter function""" - updated_setting_lists = [] - for setting_list in setting_lists: - if setting_list and setting_list[0][0] == 'flavor': - # Apply the filter function to flavors - filtered_flavors = [(k, v) for k, v in setting_list if flavor_filter(v)] - if filtered_flavors: - updated_setting_lists.append(filtered_flavors) - else: - updated_setting_lists.append(setting_list) - setting_lists[:] = updated_setting_lists - # Get simple flavors🤖 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 `@projects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.py` around lines 324 - 335, The file defines a helper function filter_flavors that is never used in BaselineComparisonsReport.do_plot while that method instead uses an inline "if/continue" flavor check; either delete the unused filter_flavors function to remove dead code, or replace the inline flavor skip in BaselineComparisonsReport.do_plot with a call to filter_flavors(setting_lists, flavor_filter) so behavior matches IntelligentRoutingComparisonsReport and PDComparisonsReport—update any variable names to match the existing signature and ensure setting_lists is mutated in place as filter_flavors does.
🤖 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 `@projects/llm-d/testing/config.yaml`:
- Line 7: The config currently forces the azure-specific preset by setting
to_apply: [azure_h100], which causes all runs to inherit Azure behavior
(including the kpouget-dev namespace); remove or unset the global to_apply entry
so the default config is neutral and require selecting the azure_h100 preset
explicitly in CI/job inputs (or move the preset into the specific job/pipeline
entries that need it) to avoid leaking Azure-specific settings into non-Azure
workflows.
- Line 138: The root-level "skip: true" toggle in
projects/llm-d/testing/config.yaml is globally scoped and will disable baseline
prepare steps for all presets; move these Azure-specific skip flags out of the
top-level prepare.*.skip and instead set them only inside the Azure CI preset
(e.g., ci_presets.azure_h100 -> prepare.*.skip) so defaults remain
unchanged—update the three occurrences noted (lines referenced in the review) by
removing or setting them false at root and adding the skip:true entries within
the ci_presets.azure_h100 overrides for the corresponding prepare sections.
---
Nitpick comments:
In
`@projects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.py`:
- Around line 324-335: The file defines a helper function filter_flavors that is
never used in BaselineComparisonsReport.do_plot while that method instead uses
an inline "if/continue" flavor check; either delete the unused filter_flavors
function to remove dead code, or replace the inline flavor skip in
BaselineComparisonsReport.do_plot with a call to filter_flavors(setting_lists,
flavor_filter) so behavior matches IntelligentRoutingComparisonsReport and
PDComparisonsReport—update any variable names to match the existing signature
and ensure setting_lists is mutated in place as filter_flavors does.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7ee7db4-319b-46ac-96f9-5edcb3d1ede1
📒 Files selected for processing (5)
projects/llm-d/testing/config.yamlprojects/llm-d/toolbox/llmd_capture_isvc_state/tasks/main.ymlprojects/llm-d/toolbox/llmd_deploy_llm_inference_service/tasks/main.ymlprojects/llm-d/visualizations/llmd_inference/data/plots.yamlprojects/llm-d/visualizations/llmd_inference/plotting/throughput_comparisons.py
kpouget
force-pushed
the
aks
branch
3 times, most recently
from
ec5f6fe to
ad0254c
Compare
|
/test jump-ci llm-d azure_h100 baseline-flavors |
|
🟢 Test of 'llm-d test test_ci' succeeded after 00 hours 20 minutes 16 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 baseline-flavors |
|
🟢 Test of 'llm-d test test_ci' succeeded after 00 hours 21 minutes 40 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 baseline-flavors guidellm_heterogeneous_eval |
|
🟢 Test of 'llm-d test test_ci' succeeded after 03 hours 21 minutes 21 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 intelligentrouting-flavors guidellm_heterogeneous_eval |
|
/test jump-ci llm-d azure_h100 baseline-flavors guidellm_heterogeneous_eval llama-70b |
|
🔴 Test of 'llm-d test test_ci' failed after 00 hours 00 minutes 40 seconds. 🔴 • Link to the test results. • No reports index generated... Test configuration: Failure indicator: Empty. (See run.log) |
|
🟢 Test of 'llm-d test test_ci' succeeded after 02 hours 16 minutes 44 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 baseline-flavors llama-70b |
|
🔴 Test of 'llm-d test test_ci' failed after 00 hours 23 minutes 44 seconds. 🔴 • Link to the test results. • Link to the reports index. Test configuration: |
|
/test jump-ci llm-d azure_h100 baseline-flavors llama-70b |
|
/test jump-ci llm-d azure_h100 baseline-flavors llama-70b |
|
🟢 Test of 'llm-d test test_ci' succeeded after 00 hours 08 minutes 02 seconds. 🟢 • Link to the test results. • Link to the reports index. Test configuration: |
1 participant
Summary by CodeRabbit
Release Notes
Chores
Bug Fixes