Skip to content

fix(zamba): add use_associative_scan config flag to avoid torch.compile slowdown - #48331

Merged
vasqu merged 2 commits into
huggingface:mainfrom
msnliu:fix/zamba-use-associative-scan
Sep 23, 2026
Merged

vasqu merged 2 commits into
huggingface:mainfrom
msnliu:fix/zamba-use-associative-scan

Conversation

@msnliu

@msnliu msnliu commented Aug 26, 2026 •

Copy link
Copy Markdown
Contributor

CPU CI GPU run-slow

ZambaMambaMixer.forward() passes use_associative_scan=False directly to mamba_selective_scan, which falls back to a sequential Python loop that makes torch.compile slow.

The other Mamba-family models (mamba, jamba, falcon_mamba) instead read this setting from their config and forward it to the scan function. This PR adds the same plumbing to ZambaConfig and ZambaMambaMixer.

Changes:

  1. configuration_zamba.py: add use_associative_scan: bool = True field
  2. modeling_zamba.py: read config.use_associative_scan in init and pass it in forward() instead of the hardcoded False

@vasqu

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

Could we have a small fast test to check this in zamba + can you fix the linter (make style)

@msnliu
msnliu force-pushed the fix/zamba-use-associative-scan branch from 48840ec to 67e232a Compare August 27, 2026 07:33
@msnliu

msnliu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Could we have a small fast test to check this in zamba + can you fix the linter (make style)

Sure, thank you for your prompt review.

@msnliu
msnliu force-pushed the fix/zamba-use-associative-scan branch from 67e232a to 3546841 Compare August 27, 2026 07:57
@msnliu

msnliu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Could we have a small fast test to check this in zamba + can you fix the linter (make style)

After looking into it, it seems that run_common_tests() already covers this via config save/reload round-trip — it didn't catch it before because the field didn't exist yet. I also checked other Mamba-family models for consistency, and they rely on run_common_tests() for this as well. The trailing whitespace has also been fixed from make style. Thank you for your time reviewing this.

@vasqu

vasqu commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Ah no for test I meant like running associative vs non associative scan and check that the logits are close enough

@msnliu

msnliu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Ah no for test I meant like running associative vs non associative scan and check that the logits are close enough

Got it, I see what you mean now — mamba has test_associative_scan_matches_sequential for this. Zamba didn't have one. I'll add the equivalent test and run the comparison. Thanks for the clarification!

@msnliu
msnliu force-pushed the fix/zamba-use-associative-scan branch from 3546841 to 44f520a Compare August 27, 2026 11:28
@msnliu

msnliu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

CI recap

Dashboard: View test results in Grafana Latest run: 33051927474:2 Result: failure | Jobs: 5 | Tests: 342 | Failures: 0 | Duration: 42s

Code quality check failed: test jobs were skipped. Fix the code quality issues and push again to run tests.

Test passed locally (27min15secs, test_associative_scan_matches_sequential), make style shows no issues in the changed files. Thanks for reviewing it again.

@msnliu
msnliu force-pushed the fix/zamba-use-associative-scan branch from 44f520a to 5ad96df Compare August 27, 2026 13:19
@msnliu

msnliu commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

@vasqu, when you have a chance, could you please review the follow-up for this PR? I'd appreciate your guidance on whether the proposed changes align with the expectations of the Hugging Face community.

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

Running the slow tests in a sec but would avoid the del model before merge

output = model.generate(input_ids, do_sample=False, use_cache=False, max_new_tokens=10)
output_text = self.tokenizer.decode(output[0].tolist())

self.assertEqual(output_text, expected_text)

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.

I guess this mimics the mamba1 test. SGTM

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. I compared the implementation when I drafted this PR.

@vasqu

vasqu commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

run-slow: zamba

@github-actions

Copy link
Copy Markdown
Contributor

Nvidia CI

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs on Nvidia:

models: ["models/zamba"]
quantizations: []

@github-actions

Copy link
Copy Markdown
Contributor

AMD CI

Workflow Run ⚙️

This comment contains run-slow, running the specified jobs on AMD:

models: ["models/zamba"]

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

@github-actions

Copy link
Copy Markdown
Contributor

CI Results (AMD)

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 6ea5acb9 workflow commit (merge commit)
PR 5ad96df0 branch commit (from PR)
main 66880ecc base commit (on main)

Model CI Report

❌ 1 new failed tests from this PR 😭

  • zamba:
    tests/models/zamba/test_modeling_zamba.py::ZambaModelTest::test_sdpa_can_compile_dynamic (✅ ⟹ ❌)

@github-actions

Copy link
Copy Markdown
Contributor

CI Results (Nvidia)

Workflow Run ⚙️

Commit Info

Context Commit Description
RUN 6ea5acb9 workflow commit (merge commit)
PR 5ad96df0 branch commit (from PR)
main 66880ecc base commit (on main)

✅ No failing test specific to this PR 🎉 👏 !

@vasqu

vasqu commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

The AMD failures are irrelevant if you can address my last comments, I think we can merge then 🤗

avoid the del model before merge

Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

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

run-slow: zamba

@github-actions

Copy link
Copy Markdown
Contributor

CI recap

Dashboard: View test results in Grafana
Latest run: 35731761500:1
Result: success | Jobs: 1 | Tests: 89 | Failures: 11 | Duration: 57m 49s

@msnliu

msnliu commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

The AMD failures are irrelevant if you can address my last comments, I think we can merge then 🤗

Thank you so much for your prompt reply. I have addressed the issue you mentioned. Thanks again for your time and efforts in reviewing this.

@vasqu
vasqu enabled auto-merge September 23, 2026 12:24
@vasqu
vasqu added this pull request to the merge queue Sep 23, 2026
Merged via the queue into huggingface:main with commit 624b32a Sep 23, 2026
39 checks passed
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.

3 participants