Skip to content

[Observability] add model initializing duration log - #52141

Merged
simon-mo merged 5 commits into
vllm-project:mainfrom
andyxning:add_model_initializing_duration_log
Oct 1, 2026
Merged

simon-mo merged 5 commits into
vllm-project:mainfrom
andyxning:add_model_initializing_duration_log

Conversation

@andyxning

@andyxning andyxning commented Aug 13, 2026 •

Copy link
Copy Markdown
Member

Purpose

Add model initializing duration log

Test Plan

NA

Test Result

NA

Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.

@andyxning

Copy link
Copy Markdown
Member Author

@22quinn

@andyxning
andyxning marked this pull request as ready for review August 24, 2026 16:52
@andyxning
andyxning requested a review from 22quinn as a code owner August 24, 2026 16:52

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@andyxning

Copy link
Copy Markdown
Member Author

Friendly ping @22quinn

@mergify

mergify Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @andyxning.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Sep 13, 2026
Signed-off-by: Andy Xie <andy.xning@gmail.com>
@andyxning
andyxning force-pushed the add_model_initializing_duration_log branch from 8777545 to 1a9738f Compare September 21, 2026 15:52
@mergify mergify Bot removed the needs-rebase label Sep 21, 2026
@andyxning

Copy link
Copy Markdown
Member Author

Friendly ping. @22quinn

@simon-mo simon-mo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI-assisted review performed on behalf of Simon Mo. Reviewed commit 8e3b88746cf08ae555ee18e560e4b53dfd2f42f1.

No blocking findings. The timer brackets model construction only, uses a monotonic clock, and preserves the weight-loading/post-processing order. A focused CPU probe executed the unchanged load_model method with a controlled clock and real torch module; it confirmed the duration, call order, and returned eval-mode model. Full GPU execution was not run locally; normal CI remains required.

@simon-mo
simon-mo enabled auto-merge (squash) September 30, 2026 18:41
@github-actions github-actions Bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 30, 2026

@simon-mo simon-mo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI-assisted re-review on behalf of Simon Mo at 06199eae21b9b3462a14774f79001a50e0c2b501. Compared the current diff with the previously reviewed change: identical. Re-ran the focused CPU timing/weight-loader probes against these current-head files (3 passed). Approval remains valid; required upstream CI must pass before merge.

@simon-mo

Copy link
Copy Markdown
Collaborator

/ci run

@github-actions

Copy link
Copy Markdown

❌ This PR is 10 commits behind upstream main. Your branch must contain every commit currently on upstream main. No new CI build was started. Merge or rebase onto the latest main, then rerun /ci run. To test this branch at your own risk, use /ci run --allow-stale.

@simon-mo simon-mo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI-assisted review on behalf of Simon Mo after updating the branch against main. Reviewed c2183183c7fd31242372951e87b630bf16ea28c9; the PR diff is byte-for-byte identical to the change already reviewed and CPU-tested. No new blocking findings. Required CI remains the merge gate.

@simon-mo

Copy link
Copy Markdown
Collaborator

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #92263 for commit c2183183c7fd.

@simon-mo

Copy link
Copy Markdown
Collaborator

AI-assisted CI follow-up from Simon’s agent: both this PR and #51792 are blocked by the same existing-main test-fixture mismatch, not by the logging change. Buildkite 92263 fails all five MiniMax PP cases in tests/v1/worker/test_eagle3_aux_hidden_states_pp.py on both AMD and NVIDIA with AttributeError: ... use_v2_model_runner.

I checked base/main 8fb16ea5a66f0ef362e728f0ff025f2845927fd5: the fixture passes a SimpleNamespace without use_v2_model_runner or speculative_config, while MiniMaxM3Model.__init__ reads the former and spec_decode_needs_target_embed reads the latter. For these non-speculative PP fixtures, the proposed fix is to supply both:

vllm_config=SimpleNamespace(
    model_config=SimpleNamespace(hf_text_config=config),
    quant_config=None,
    use_v2_model_runner=False,
    speculative_config=None,
)

@andyxning could you add this fixture correction (preferably once upstream, then update both branches), and run the five existing PP tests? I’ll re-review the resulting heads, restart CI, and merge once required checks pass. A blind retry won’t resolve these deterministic failures.

Executor/worker log.

This build also has a separate API-server metrics failure: test_weight_sync_metrics_aggregate_across_api_servers got one recording PID instead of two; all operation totals passed. The test assumes eight syncs reach both workers, so request distribution is a possible cause, not yet proven. We should recheck this on the corrected build. Failing API-server job.

@simon-mo simon-mo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI-assisted re-review for Simon: refreshed against current main including the MiniMax PP fixture correction (#59508). The PR diff is byte-identical to the previously reviewed logging change. Focused CPU review probes pass; required hosted CI remains the merge gate.

@simon-mo

simon-mo commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator

/ci run

@simon-mo

simon-mo commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator

@andyxning the shared MiniMax PP fixture mismatch is now fixed upstream by #59508. I updated this branch to main, verified the PR diff is unchanged, reran the three focused logging probes, refreshed approval on the new head, and requested a new CI build. No separate fixture fix is needed from you now. I will also check whether the API-server metrics failure recurs before merging. (AI-assisted follow-up for Simon.)

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #92306 for commit 4ca22da6a8e5.

@simon-mo
simon-mo merged commit bfbcde2 into vllm-project:main Oct 1, 2026
157 checks passed
@andyxning
andyxning deleted the add_model_initializing_duration_log branch October 7, 2026 14:10
michael-gpu pushed a commit to practical-inference/vllm-ampere-extended that referenced this pull request Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants