Fix sliding_window hasattr in Mistral#27041
Fix sliding_window hasattr in Mistral#27041ArthurZucker merged 2 commits intohuggingface:mainfrom IlyaGusev:patch-1
Conversation
|
One more possible way to fix it is to use |
|
Just to better understand, how can |
|
@patrickvonplaten correct, it should never be |
There was a problem hiding this comment.
| and hasattr(self.config, "sliding_window") |
as per @patrickvonplaten 's explanation above
There was a problem hiding this comment.
What is the purpose of use_sliding_windows then? My understanding is that one can disable sliding windows by deleting this attribute completely.
So there are two possible cases:
- No "sliding_window" in the config -> use_sliding_windows is False -> no additional code is used
- "sliding_window" is in the config and it is not None -> use_sliding_windows is True when kv_cache is large enough -> additional code about sliding windows is used.
And there is the third case that is not possible:
3) "sliding_window" is in the config and it is None
The problem is that to reach case 1, one has to manually delattr this field.
So my suggestion is to replace hasattr(self.config, "sliding_window") with getattr(self.config, "sliding_window", None) is not None or even self.config.sliding_window is not None.
And by setting it to None, users can disable sliding windows.
|
So this way if someone wants to disable sliding windows, they should either delete the attribute or set it to None. |
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
|
I think it is still relevant and should be merged. |
ArthurZucker
left a comment
There was a problem hiding this comment.
Actually setting it to None should de-activate the sliding of the cache so yes.
Can you make sure to rebase on main?
|
Done, I've rebased. |
|
Thanks for the fix! |
What does this PR do?
It fixes a bug with checking
hasattrresult withis not None.hasattrreturnsTrueofFalse, so the condition is always True without this fix.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@ArthurZucker @younesbelkada or anyone else.