[v0.5.10][4] Fix _no_split_modules set not subscriptable in transformers >=5.0#931
[v0.5.10][4] Fix _no_split_modules set not subscriptable in transformers >=5.0#931yueming-yuan wants to merge 3 commits intofix/processor-return-tensorsfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the apply_fsdp2 function to handle _no_split_modules as either a list or a set, ensuring compatibility with different versions of the transformers library. The reviewer suggested explicitly converting layer_cls_to_wrap to a set to improve the efficiency of membership checks in subsequent operations.
|
|
||
| layer_cls_to_wrap = model._no_split_modules | ||
| assert len(layer_cls_to_wrap) > 0 and layer_cls_to_wrap[0] is not None | ||
| assert len(layer_cls_to_wrap) > 0 and next(iter(layer_cls_to_wrap)) is not None |
There was a problem hiding this comment.
While next(iter(layer_cls_to_wrap)) correctly handles the compatibility between list and set types for _no_split_modules in transformers >= 5.0, it is more idiomatic and efficient to ensure layer_cls_to_wrap is a set once at the beginning. This would also optimize the membership check in the subsequent loop (line 689) for cases where transformers < 5.0 provides a list (making it
| assert len(layer_cls_to_wrap) > 0 and next(iter(layer_cls_to_wrap)) is not None | |
| layer_cls_to_wrap = set(model._no_split_modules) | |
| assert layer_cls_to_wrap and next(iter(layer_cls_to_wrap)) is not None |
Summary
model._no_split_moduleschanged fromlisttosetin transformers >=5.0setdoesn't support[0]indexing, causingTypeError: 'set' object is not subscriptableTest plan