Skip to content

[Bugfix][Metrics][MFU] Size activation traffic from the model dtype - #57070

Merged
markmc merged 6 commits into
vllm-project:mainfrom
thillai-c:fix/mbu-activation-dtype
Sep 17, 2026
Merged

markmc merged 6 commits into
vllm-project:mainfrom
thillai-c:fix/mbu-activation-dtype

Conversation

@thillai-c

@thillai-c thillai-c commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

BaseConfigParser sizes weight traffic from the model's dtype but pins activation traffic to 2 bytes:

args.weight_byte_size = get_dtype_size(torch_dtype)

# FIXME: handle this better by parsing whether activations use
# bf16, fp32, etc...
args.activation_byte_size = 2

torch_dtype is already resolved on the line above, so the two values come from the same config and disagree. For a model served with --dtype float32, every activation term in the MBU accounting is counted at half its real size while that same model's weight terms are correct. Activations are produced in the model's compute dtype, so this keys them on it.

activation_byte_size is set once here and never overridden. Quantization parsers override weight_byte_size (fp8, mxfp4 and friends) but leave activations alone, which is why the model dtype remains the right source after this change.

Impact

  • bfloat16 and float16: unchanged, 2 bytes, which covers the overwhelming majority of deployments.
  • float32: 2 to 4 bytes. Activation read and write traffic doubles and reported MBU drops accordingly.

The float32 change is user visible. Anyone tracking MBU for a float32 deployment will see the number move; that movement is the correction, since the previous value understated memory traffic.

Test Plan

pytest -v tests/v1/metrics/test_perf_metrics.py
pre-commit run ruff-check ruff-format --files \
  vllm/v1/metrics/perf.py tests/v1/metrics/test_perf_metrics.py

New tests in the existing file:

  • test_base_config_parser_activation_byte_size_follows_dtype, parametrized over float16, bfloat16 and float32, asserting activation and weight byte sizes agree with the dtype.
  • test_attention_read_bytes_scale_with_activation_dtype, which goes through AttentionMetrics.get_read_bytes_breakdown and asserts qkv_input, a pure activation term, is exactly twice as large for float32 as for bfloat16.

The existing test_base_config_parser assertion of activation_byte_size == 2 for float16 still holds and is unchanged; only its comment was updated, since 2 is now the dtype's size rather than a default.

Test Result

72 passed, 1 warning in 19.45s

That is the 68 pre-existing tests plus the 4 added here.

Linting:

ruff check...............................................................Passed
ruff format..............................................................Passed

No model evaluation: this changes analytic MFU/MBU accounting only. No kernel, no model computation, and no output logits are affected.

AI assistance

AI assistance (Claude Code) was used to locate this and draft the tests. I reviewed every changed line and ran the commands above myself.


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.

@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.

@mergify mergify Bot added the bug Something isn't working label Sep 15, 2026
@thillai-c

Copy link
Copy Markdown
Contributor Author

@markmc mind taking a look? float32 models under-report activation traffic because activation_byte_size was hardcoded to 2. Resolves the FIXME at perf.py:383.

@markmc markmc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, thanks

@markmc markmc added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 16, 2026
@markmc

markmc commented Sep 16, 2026

Copy link
Copy Markdown
Member

/ci run

@github-actions

Copy link
Copy Markdown

✅ @thillai-c, CI is now available for this PR.

  • /ci run starts upstream CI; /amd-ci run starts AMD CI only.
  • Your branch must contain every commit currently on its upstream target branch. Merge or rebase onto the latest target branch, then rerun the command. Append --allow-stale to a run command to test an outdated branch at your own risk.
  • /ci retry retries failed jobs in the CI build for the current PR head. If the current head has no CI build, it starts a new CI build for the current head containing only jobs that failed in the latest earlier CI build for this PR.
  • /amd-ci retry retries failed jobs in AMD CI for the current PR head. Use /amd-ci run when the current head has no AMD CI build.
  • /ci cancel cancels scheduled or running CI builds for this PR branch; /amd-ci cancel does the same for AMD CI only.

Signed-off-by: Thillai Chithambaram <thillaichithambaram.a@gmail.com>
@markmc
markmc force-pushed the fix/mbu-activation-dtype branch from 414b832 to 054689b Compare September 16, 2026 10:25
@markmc
markmc enabled auto-merge (squash) September 16, 2026 10:25
@markmc markmc changed the title [Bugfix][Metrics] Size activation traffic from the model dtype [Bugfix][Metrics][MFU] Size activation traffic from the model dtype Sep 16, 2026
@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #89327 for commit 054689b85f4c.

@mergify

mergify Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Hi @thillai-c, the pre-commit checks have failed. Please run:

uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-files

Then, commit the changes and push to your branch.

For future commits, pre-commit will run automatically on changed files before each commit.

@thillai-c

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

Copy link
Copy Markdown

❌ This PR is 2 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.

@thillai-c

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #89388 for commit 5e4c80e53c1b.

@mergify

mergify Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Hi @thillai-c, the pre-commit checks have failed. Please run:

uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-files

Then, commit the changes and push to your branch.

For future commits, pre-commit will run automatically on changed files before each commit.

@thillai-c

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #89400 for commit d431029efa7e.

@thillai-c

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #89449 for commit b351726d1d5d.

@markmc
markmc merged commit a615f53 into vllm-project:main Sep 17, 2026
116 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in Prometheus Metrics Sep 17, 2026
michael-gpu pushed a commit to practical-inference/vllm-ampere-extended that referenced this pull request Oct 20, 2026
…llm-project#57070)

Signed-off-by: Thillai Chithambaram <thillaichithambaram.a@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready ONLY add when PR is ready to merge/full CI is needed

Projects

Development

Successfully merging this pull request may close these issues.

2 participants