[Bugfix]: Correct handling of cos_sin_cache length #1900
Conversation
Signed-off-by: Jade Zheng <zheng.shoujian@outlook.com>
|
@whx-sjtu PTAL |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes performance issues in rotary embedding cos/sin cache handling by correcting variable usage and preventing unnecessary cache recreation. The fix ensures that the cache, which is already initialized with maximum context length, is not unnecessarily recreated during processing.
- Replaces cache recreation logic with an error when max_seq_len exceeds the initialized maximum
- Corrects variable assignment in
_set_cos_sin_cachefrommax_seq_len_cachedtomax_seq_len - Removes redundant
max_seq_lenassignment during initialization since the cache setup handles this
…alid inputs Signed-off-by: Jade Zheng <zheng.shoujian@outlook.com>
jianzs
added
performance-test
Signed-off-by: Jade Zheng <zheng.shoujian@outlook.com>
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (75.00%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1900 +/- ##
==========================================
+ Coverage 60.17% 60.21% +0.04%
==========================================
Files 71 71
Lines 7989 7995 +6
==========================================
+ Hits 4807 4814 +7
+ Misses 3182 3181 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@whx-sjtu Which PR fixed this issue in the 0.9.1-dev branch? |
| @@ -209,7 +211,7 @@ def apply_rotary_pos_emb(q, k, cos, sin, position_ids, unsqueeze_dim=1): | |||
|
|
|||
|
|
|||
| def _set_cos_sin_cache(self, seq_len, device, dtype): | |||
| self.max_seq_len_cached = seq_len | |||
| self.max_seq_len = seq_len * self.scaling_factor | |||
There was a problem hiding this comment.
There is no problem in v0.9.1. what happens.
There was a problem hiding this comment.
#1551 fixed this problem in v0.9.1-dev
What this PR does / why we need it?
This PR addresses the performance issue related to cos/sin cache handling:
The cos/sin cache is already initialized with the maximum context length during initialization. However, due to
max_seq_len_cachebeing stored asseq_len, the condition check was incorrect, leading to unnecessary cache recreation.Since the cos/sin cache is already initialized with maximum context length, it should not trigger recreation during the process.
Fixed variable naming:
max_seq_len_cachewas never used and should bemax_seq_len. This also is the correct variable to check against the maximum context length.Does this PR introduce any user-facing change?
No
How was this patch tested?
CI pass.