fix(autotp): when using autotp, ignore the consistency of certain data within the tp_group.#8125
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a865dd365c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@1787648106 thanks for your fix. I agree with codex that the import from transformers should be catched. If there is import error, then the tensor won't be HF model KV cache. This would avoid break existing use cases. Also can you sign-off your commits to pass DCO check? Thanks! |
Done, thank you! |
…tp_group Signed-off-by: lunzhongwang <1787648106@qq.com>
Signed-off-by: lunzhongwang <1787648106@qq.com>
Signed-off-by: lunzhongwang <1787648106@qq.com>
4d08564 to
fbfe8fd
Compare
|
Hi @1787648106 can you fix the formatting errors? You may want to refer to this link for essential pre CI checks. Thanks! |
Signed-off-by: lunzhongwang <1787648106@qq.com>
done |
There was a problem hiding this comment.
Hi @1787648106,
Thank you for submitting this PR, great catch! I left a comment below. Happy to hear you thought.
| def is_transformers_cache(obj): | ||
| """Skip checks for the `transformers` cache class when performing AutoTP tensor comparisons.""" | ||
| try: | ||
| from transformers.cache_utils import (DynamicCache, StaticCache, QuantizedCache) |
There was a problem hiding this comment.
The transformers versions supporting these cache classes are different. So is_transformers_cache(dynamic_cache) can return False. Also, we have cache classes not listed here, e.g. SinkCache.
Do you think we can check this by the parent class like:
def is_transformers_cache(obj):
try:
from transformers.cache_utils import Cache
except ImportError:
return False
return isinstance(obj, Cache)
When using
autotp, the consistency check for the KV cache within the TP group is bypassed. Recent popular methods, such as on-policy distillation, require the model to generate output during training, which would otherwise cause the data consistency check to fail.