Set return_dict=True and remove output_hidden_states in Transformer - #7
Set return_dict=True and remove output_hidden_states in Transformer#7arthrod wants to merge 10 commits into
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR modifies the encoder’s forward pass to stop explicitly requesting hidden states from the underlying model (relying on the model’s default behavior instead) and includes minor formatting adjustments in two files. Class diagram for encoder and its interaction with underlying modelclassDiagram
class Encoder {
+config
+model
+layers_fuser
+forward(*args, **kwargs)
+encode_text(input_ids, attention_mask, *args, **kwargs)
+encode_labels(labels_input_ids, labels_attention_mask, *args, **kwargs)
}
class UnderlyingModel {
+__call__(*args, output_hidden_states, return_dict, **kwargs)
}
class LayersFuser {
+__call__(hidden_states)
}
Encoder --> UnderlyingModel : uses
Encoder --> LayersFuser : uses when config.fuse_layers
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @arthrod, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily adjusts a model call by commenting out the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThis PR applies minimal changes across two files. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Commenting out
output_hidden_statesin the call toself.modelwhile still computingoutput_hidden_statesabove leaves an unused variable and may breakfuse_layerslogic that depends onhidden_states; either remove the variable and any dependent code or keep passing the flag explicitly. - The inline comment
#output_hidden_states = output_hidden_statesinside the argument list makes the call harder to read; if this is intentionally disabled, prefer removing the argument entirely or adding a clearer comment above the call explaining why hidden states are no longer requested.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Commenting out `output_hidden_states` in the call to `self.model` while still computing `output_hidden_states` above leaves an unused variable and may break `fuse_layers` logic that depends on `hidden_states`; either remove the variable and any dependent code or keep passing the flag explicitly.
- The inline comment `#output_hidden_states = output_hidden_states` inside the argument list makes the call harder to read; if this is intentionally disabled, prefer removing the argument entirely or adding a clearer comment above the call explaining why hidden states are no longer requested.
## Individual Comments
### Comment 1
<location> `gliner/modeling/encoder.py:100-101` </location>
<code_context>
else:
output_hidden_states = False
- output = self.model(*args, output_hidden_states = output_hidden_states,
+ output = self.model(*args, #output_hidden_states = output_hidden_states,
return_dict = True, **kwargs)
if self.config.fuse_layers:
encoder_layer = self.layers_fuser(output.hidden_states)
</code_context>
<issue_to_address>
**issue (bug_risk):** Passing `output_hidden_states` is commented out but related logic is still active, which may break `fuse_layers` behavior.
Since `output_hidden_states` is still derived from `self.config.fuse_layers` but no longer passed to `self.model`, `output.hidden_states` may be absent if the model’s default is `output_hidden_states=False`, breaking or degrading `self.layers_fuser`. Either remove the now-dead `output_hidden_states` logic and explicitly handle the `fuse_layers` path, or restore/gate the argument instead of commenting it out in place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| output = self.model(*args, #output_hidden_states = output_hidden_states, | ||
| return_dict = True, **kwargs) |
There was a problem hiding this comment.
issue (bug_risk): Passing output_hidden_states is commented out but related logic is still active, which may break fuse_layers behavior.
Since output_hidden_states is still derived from self.config.fuse_layers but no longer passed to self.model, output.hidden_states may be absent if the model’s default is output_hidden_states=False, breaking or degrading self.layers_fuser. Either remove the now-dead output_hidden_states logic and explicitly handle the fuse_layers path, or restore/gate the argument instead of commenting it out in place.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to adjust how output_hidden_states is handled in the encoder. However, the current change of commenting out the output_hidden_states parameter is likely to introduce a bug. When layer fusion is enabled, the model needs to output hidden states, but with this change, it will probably not, leading to a runtime error. I've provided a critical review comment with a suggested fix that not only corrects the issue but also simplifies the related code. The other changes are minor stylistic improvements, like adding newlines at the end of files, which are good practices.
| else: | ||
| output_hidden_states = False | ||
| output = self.model(*args, output_hidden_states = output_hidden_states, | ||
| output = self.model(*args, #output_hidden_states = output_hidden_states, |
There was a problem hiding this comment.
This change introduces a potential bug. By commenting out output_hidden_states, the model will rely on its default configuration. If self.config.fuse_layers is True (checked on line 96), line 103 accesses output.hidden_states, which will likely not be present in the model's output, causing an AttributeError at runtime. The output_hidden_states parameter must be explicitly set to True when layer fusion is enabled.
Instead of commenting out this line, the logic should be corrected and simplified. The entire block from line 96 to 101 can be replaced with:
output = self.model(
*args,
output_hidden_states=self.config.fuse_layers,
return_dict=True,
**kwargs
)This is more concise and ensures output_hidden_states is correctly passed when needed.
coderabbitai
Bot
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e017e04117
ℹ️ 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| output = self.model(*args, #output_hidden_states = output_hidden_states, | ||
| return_dict = True, **kwargs) |
There was a problem hiding this comment.
When fuse_layers is enabled, this forward path still computes output_hidden_states = True but no longer forwards it to the transformer call, so output.hidden_states is typically None for standard HF configs and self.layers_fuser(output.hidden_states) will fail at runtime (or produce invalid fusion input) for any run using layer fusion. This change effectively breaks the fuse_layers feature introduced in this module.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gliner/modeling/encoder.py (1)
100-105:⚠️ Potential issue | 🔴 CriticalCritical bug:
output_hidden_statesis computed but never passed to the model.When
fuse_layersis enabled, the code setsoutput_hidden_states = True(line 97) but the parameter is commented out in the model call (line 100). This meansoutput.hidden_stateson line 103 will beNone, causing thefuse_layersfeature to fail at runtime.The commented-out argument should be restored.
🐛 Proposed fix
- output = self.model(*args, `#output_hidden_states` = output_hidden_states, - return_dict = True, **kwargs) + output = self.model(*args, output_hidden_states=output_hidden_states, + return_dict=True, **kwargs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gliner/modeling/encoder.py` around lines 100 - 105, The model call is not receiving the computed output_hidden_states flag so output.hidden_states is None when self.config.fuse_layers is true; restore the argument by passing output_hidden_states=output_hidden_states into the self.model(...) call (alongside return_dict=True and **kwargs) so that output.hidden_states is populated for layers_fuser; update the call in encoder where self.model is invoked and remove the commented-out parameter to ensure layers_fuser(output.hidden_states) can run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@gliner/modeling/encoder.py`:
- Around line 100-105: The model call is not receiving the computed
output_hidden_states flag so output.hidden_states is None when
self.config.fuse_layers is true; restore the argument by passing
output_hidden_states=output_hidden_states into the self.model(...) call
(alongside return_dict=True and **kwargs) so that output.hidden_states is
populated for layers_fuser; update the call in encoder where self.model is
invoked and remove the commented-out parameter to ensure
layers_fuser(output.hidden_states) can run.
|
@coderabbitai review carefully again after these edits |
|
✅ Actions performedReview triggered.
|
2 participants
Summary by Sourcery
Bug Fixes: