-
-
Notifications
You must be signed in to change notification settings - Fork 11k
[Model] Add support for ModernBertForTokenClassification #26340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Model] Add support for ModernBertForTokenClassification #26340
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run You ask your reviewers to trigger select CI tests on top of Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds support for ModernBertForTokenClassification. The changes include the model implementation, registration, and a new test case. The implementation of ModernBertForTokenClassification is missing the application of the dropout layer, which is a correctness issue. Additionally, the new test case for this model has a bug in its result-checking loop that will cause it to fail. I've provided suggestions to fix both of these critical issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
| # check logits difference | ||
| for hf_output, vllm_output in enumerate(zip(hf_outputs, vllm_outputs)): | ||
| hf_output = torch.tensor(hf_output).cpu().float() | ||
| vllm_output = torch.tensor(vllm_output).cpu().float() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token classification test compares index tensor to model output
The ModernBERT token classification test iterates with enumerate(zip(hf_outputs, vllm_outputs)), so hf_output is the loop index and vllm_output is a tuple of tensors. The subsequent conversion to tensors (torch.tensor(hf_output) / torch.tensor(vllm_output)) therefore attempts to turn an integer and a tuple of tensors into tensors, raising a TypeError before the assertion can run. This means the new test fails regardless of model correctness. The loop should iterate over zip(hf_outputs, vllm_outputs) directly so that the actual logits are compared.
Useful? React with 👍 / 👎.
|
@noooop would be great if you could help review this! |
| inputs_embeds=inputs_embeds, | ||
| intermediate_tensors=intermediate_tensors, | ||
| ) | ||
| hidden_states = self.drop(self.head(hidden_states)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution.
Clear and clean, with only a few details needing modification.
For the vLLM inference framework, there is no need to implement dropout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I have made a commit to remove the dropout layer from the model, and dismiss its weight loading in load_weights.
1699421 to
303edff
Compare
|
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks both of you!
|
Actually, can you also update the Supported Models page |
Signed-off-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: antrec <antoine.recanati@gmail.com>
Added dropout layer in forward, which should not have effect at inference, though, but I still kept it in case, and to match huggingface architecture so that weight loading is simplified. Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: antrec <antoine.recanati@gmail.com>
Signed-off-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr>
Signed-off-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr>
1c21935 to
5a839fd
Compare
…t#26340) Signed-off-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Signed-off-by: antrec <antoine.recanati@gmail.com> Co-authored-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
The test fails in https://buildkite.com/vllm/ci/builds/33941/steps/canvas?sid=0199c1fa-d5a7-40a2-8f02-b45545380a3b, can you fix it? |
fix in #26414 |
Thanks! So, there is nothing to do on my side? The test passed locally for me before committing, but I did not manage to have the exact same setup than in the CI on my Macbook (I did not use |
There are minor differences in numerical precision across different devices. It’s okay; this has been fixed by #26466. |
…t#26340) Signed-off-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Signed-off-by: antrec <antoine.recanati@gmail.com> Co-authored-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
…t#26340) Signed-off-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Signed-off-by: antrec <antoine.recanati@gmail.com> Co-authored-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Dhruvil Bhatt <bhattdbh@amazon.com>
…t#26340) Signed-off-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Signed-off-by: antrec <antoine.recanati@gmail.com> Co-authored-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…t#26340) Signed-off-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Signed-off-by: antrec <antoine.recanati@gmail.com> Co-authored-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…t#26340) Signed-off-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Signed-off-by: antrec <antoine.recanati@gmail.com> Co-authored-by: Antoine Recanati Le Goat <antoine.recanati@sancare.fr> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Purpose
Add support for ModernBertForTokenClassification.
I got inspired from #24872 , and adapted to ModernBert architecture. I did not touch to flex attention backend or anything, though, because I expected the changes already done in that previous PR to make BERT work for NER, would work for ModernBert as well.
Test Plan
Added a single test case in
tests/models/language/pooling/test_token_classification.py, that checks the results of huggingface and vllm runners for disham993/electrical-ner-ModernBERT-base are almost equal. I took a "random" model, as little as possible (there are not many modernbert for NER available on huggingface).Test Result
The test passes.
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.