Skip to content

Finegrained FP8 v4: fused grouepd/batched - #46957

Open
IlyasMoutawwakil wants to merge 8 commits into
mainfrom
finegrained-fp8-v4
Open

Finegrained FP8 v4: fused grouepd/batched#46957
IlyasMoutawwakil wants to merge 8 commits into
mainfrom
finegrained-fp8-v4

Conversation

@IlyasMoutawwakil

@IlyasMoutawwakil IlyasMoutawwakil commented Jun 29, 2026

Copy link
Copy Markdown
Member

CI

What does this PR do?

download (2)

Code Agent Policy

The Transformers repo is currently being overwhelmed by a large number of PRs and issue comments written by
code agents. We are currently bottlenecked by our ability to review and respond to them. As a result,
we ask that new users do not submit pure code agent PRs at this time.
You may use code agents in drafting or to help you diagnose issues. We'd also ask autonomous "OpenClaw"-like agents
not to open any PRs or issues for the moment.

PRs that appear to be fully agent-written will probably be closed without review, and we may block users who do this
repeatedly or maliciously.

This is a rapidly-evolving situation that's causing significant shockwaves in the open-source community. As a result,
this policy is likely to be updated regularly in the near future. For more information, please read CONTRIBUTING.md.

  • I confirm that this is not a pure code agent PR.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline and the
    Pull Request checks?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes according to the guidelines?
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@IlyasMoutawwakil
IlyasMoutawwakil requested review from ArthurZucker, Copilot and vasqu and removed request for vasqu June 29, 2026 17:35
@IlyasMoutawwakil
IlyasMoutawwakil marked this pull request as ready for review June 29, 2026 17:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the finegrained-fp8 kernel integration to v4 and extends FP8 MoE support with new fused grouped/batched expert dispatch paths, while also refining decode-time experts implementation switching based on batch size.

Changes:

  • Bump the finegrained-fp8 kernel mapping to version 4 and extend the Python wrapper to expose new v4 entry points (including fused MoE ops).
  • Add new fused MoE expert forward implementations (fused_grouped_mm, fused_batched_mm) and register them in the experts interface mapping.
  • Update generation decode optimization to conditionally switch *_grouped_mm -> *_batched_mm only for small batches (<= 32), and update call sites to pass batch_size.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/transformers/models/higgs_audio_v2/generation_higgs_audio_v2.py Passes batch_size into the updated _optimize_model_for_decode context manager.
src/transformers/integrations/hub_kernels.py Bumps finegrained-fp8 kernel mapping version from 3 → 4.
src/transformers/integrations/finegrained_fp8.py Updates kernel API bindings to v4 naming and adds fused MoE expert dispatch implementations.
src/transformers/generation/utils.py Makes decode-time experts switching batch-size aware and updates _sample() to pass batch_size.

Comment thread src/transformers/integrations/finegrained_fp8.py

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

Some smaller comments but looks overall good!

Comment thread src/transformers/integrations/finegrained_fp8.py
"any issues with this, please open an issue on the Hugging Face Transformers GitHub repository.",
)
self.set_experts_implementation("batched_mm")
self.set_experts_implementation(decode_experts_implementation)

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.

It looks a bit weird with the variables now, can we add a nudge comment that this is to note for fp8

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

its more general where each grouped has a batched alternative

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.

Yep yep, just a small comment to note this is to cover all grouped mm etc cases including fp8

)

finegrained_fp8 = load_finegrained_fp8_kernel()
fused_op = getattr(finegrained_fp8, op_name)

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.

feels a bit unsafe :D

raise NotImplementedError(
f"{op_name} experts dispatch requires gated experts (a combined gate_up projection)."
)
if self.swiglu_alpha is not None or self.swiglu_limit is not None or self.limit is not None:

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.

hmm didnt we ignore it for the base grouped mm etc? Why make an exception here then

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no we didn't actually ignore it in base grouped, we normally have _apply_gate which applies these in between grouped gemms, i guess you mean sonicmoe or some other backend.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

imo better have an error that nudges users to open a pr in kernels-community rather than wrong outputs

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.

Ah oops yes it was sonicmoe I think. But yes agree, probably better to error out early

Should we introduce something similar for sonicmoe then?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yess will do

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

do you have any bench you can publish in this PR desc before merging?

@IlyasMoutawwakil

Copy link
Copy Markdown
Member Author

was actually making them when i realized m3 has swiglu alpha/limlit 😅 publishing after kernels pr merge

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: deepseek_v4, gpt_oss, higgs_audio_v2, minimax_m3_vl, openai_privacy_filter, finegrained_fp8

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

CI recap

Dashboard: View test results in Grafana
Latest run: 28534097273:1
Result: success | Jobs: 13 | Tests: 46,605 | Failures: 0 | Duration: 18h 24m

@IlyasMoutawwakil

Copy link
Copy Markdown
Member Author

did some changes, along deprecating alpha and limit

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

Just some smaller comments but overall LGTM



@functools.cache
_DEEPGEMM: DeepGEMM | None = None

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.

Ah it was still loading multiple times? lazy load kernels should pretty much interact as cache already. Maybe we should rather cache that we are compatible device wise etc

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no no it was loading once, it just thrashes the stdout with dynamo warnings about it

matmul_batched: Callable
matmul_grouped: Callable
moe_fused_grouped: Callable
moe_fused_batched: Callable

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.

This is kind of public facing, should we add a 🚨 just in case? Usually people shouldnt use it but you never know

Comment on lines +93 to +95
Cached in the module-global ``_FINEGRAINED_FP8`` (populated on first call) instead of via
``functools.cache``: Dynamo traces through the lru wrapper and warns on every compiled call,
whereas a plain global check/return traces cleanly.

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.

Ah this explanation was missing in deepgemm

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes

self.swiglu_alpha = getattr(config, "swiglu_alpha", None)
self.swiglu_limit = getattr(config, "swiglu_limit", None)
self.act_fn = ACT2FN[_first_attr(config, "hidden_activation", "hidden_act")]
self.limit = getattr(config, "swiglu_limit", None)

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.

need to deprecate here as well no?

raise ValueError("sonicmoe requires gated experts (has_gate=True)")
if hidden_states.device.type != "cuda":
raise ValueError("sonicmoe requires CUDA device")
if getattr(self, "swiglu_alpha", None) is not None or getattr(self, "swiglu_limit", None) is not None:

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.

Yes thanks 🙏

# Match a full module path or a trailing component (a bare name given under a submodel);
# keep the renamed entry as-is if it names no module (a regex / leaf-param pattern).
matches = [m for m in module_names if m == entry or m.endswith(f".{entry}")]
remapped.extend(matches or [entry])

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.

hmm interesting, where did you encounter this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

m3 mxfp8 config has vision_tower without the "model." prefix, not sure whats the best way to handle them tho

@vasqu vasqu mentioned this pull request Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants