-
Notifications
You must be signed in to change notification settings - Fork 34.7k
🚨 Pass tp_plan from lm_heads #47253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🚨 Pass tp_plan from lm_heads #47253
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1268,6 +1268,9 @@ class PreTrainedModel(nn.Module, EmbeddingAccessMixin, ModuleUtilsMixin, PushToH | |
| # models, this attribute is currently defined in respective model code. For base models, it comes from | ||
| # `config.base_model_pp_plan` during `post_init`. | ||
| _pp_plan: dict[str, tuple[str, str]] = None | ||
| # An expert parallel plan used instead of `_tp_plan` when expert parallelism is enabled. For base models, it comes | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not super
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you elaborate? 😁 |
||
| # from `config.base_model_ep_plan` during `post_init`. | ||
| _ep_plan: dict[str, str] = None | ||
| # FSDP2 sharding plan of the form `{"layers.*": "free_full_weight"}`. For top-level models, this attribute is | ||
| # defined on the head class (e.g. `*ForCausalLM`). For base models, it comes from `config.base_model_fsdp_plan` | ||
| # during `post_init`. | ||
|
|
@@ -1415,15 +1418,18 @@ def post_init(self): | |
| """ | ||
| # Attach the different parallel plans and tied weight keys to the top-most model, so that everything is | ||
| # easily available. | ||
| self._tp_plan, self._ep_plan, self._pp_plan, self._fsdp_plan = {}, {}, {}, {} | ||
| # Start from the class-level plans (e.g. `{"lm_head": "colwise_rep"}` on `...ForCausalLM` classes), copying | ||
| # them as they are mutated below and would otherwise contaminate the class attribute shared by all instances | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. absolutely, this was already the case.... do you know where the regression comes from?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah wrote that mostly for review. Yes, it seems to be coming from a big Revert, likely an oversight #46246
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's actually in #36677 and then we keep the pattern
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure @3outeille ? it doesn't seem to be there that we set |
||
| self._tp_plan = dict(self._tp_plan or {}) | ||
| self._ep_plan = dict(self._ep_plan or {}) | ||
| self._pp_plan = dict(self._pp_plan or {}) | ||
| self._fsdp_plan = dict(self._fsdp_plan or {}) | ||
| # If current model is a base model, attach `base_model_tp_plan` and `base_model_pp_plan` from config | ||
| if self.base_model is self: | ||
| self._pp_plan = self.config.base_model_pp_plan.copy() if self.config.base_model_pp_plan is not None else {} | ||
| self._tp_plan = self.config.base_model_tp_plan.copy() if self.config.base_model_tp_plan is not None else {} | ||
| self._ep_plan = self.config.base_model_ep_plan.copy() if self.config.base_model_ep_plan is not None else {} | ||
| self._fsdp_plan = ( | ||
| self.config.base_model_fsdp_plan.copy() if self.config.base_model_fsdp_plan is not None else {} | ||
| ) | ||
| self._pp_plan.update(self.config.base_model_pp_plan or {}) | ||
| self._tp_plan.update(self.config.base_model_tp_plan or {}) | ||
| self._ep_plan.update(self.config.base_model_ep_plan or {}) | ||
| self._fsdp_plan.update(self.config.base_model_fsdp_plan or {}) | ||
| # Current submodel should register its tied weights | ||
| self.all_tied_weights_keys = self.get_expanded_tied_weights_keys(all_submodels=False) | ||
| # Current submodel should register its `_keep_in_fp32_modules` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -491,6 +491,15 @@ def _get_tp_model_class(self): | |
| return self.model_tester.causal_lm_class | ||
| return self.all_model_classes[0] | ||
|
|
||
| def _get_tp_config(self): | ||
| """Tiny config with `vocab_size` rounded up to a multiple of the world size, as sharded dims (typically `lm_head`) have to be split across ranks.""" | ||
| config = self.model_tester.get_config() | ||
| text_config = config.get_text_config() | ||
| remainder = text_config.vocab_size % self.tensor_parallel_size | ||
| if remainder: | ||
| text_config.vocab_size += self.tensor_parallel_size - remainder | ||
| return config | ||
|
Comment on lines
+496
to
+501
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is because we have typically vocab size 99 on tiny tests, which can't be sharded on 2 ranks obviously
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm thinking wheter we should automatically extend an embedding if we notice this? So in from pretrained, if we notice
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah auto-pad you mean? hmm we could ig, I feel like we are already doing it in a couple other places IIRC
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, because users will complain I'm sure 😅 Maybe a warning but having users to think leads to problems
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but we should pad just before the gather and unpad right after, no?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. other idea (sorry for the noise): we should just validate, feels less magic. like if TP is provided with a wrong vocab size just raise from the get-go. Should be in another PR though because it could break existing setups
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is already breaking in itself no? Users could have arbitrary vocab sizes before this But yea, I can see the validation path - less magic and add how to properly do it
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah yes, you're right... before it was silently replicating so of course this will raise. I'll add the validation logic then
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, if it does not cost much and we can resize for the user, would be nice. People probably expect us to do so, and we are the ones setting the plan to default to colwise rep. |
||
|
|
||
| def _skip_if_not_supported(self, expert_parallel: bool = False): | ||
| """Check and skip the test if tensor/expert parallel is not supported for this model/environment.""" | ||
| parallelism = "Expert" if expert_parallel else "Tensor" | ||
|
|
@@ -539,7 +548,7 @@ def _skip_if_not_supported(self, expert_parallel: bool = False): | |
| def test_tp_forward(self): | ||
| self._skip_if_not_supported() | ||
|
|
||
| config = self.model_tester.get_config() | ||
| config = self._get_tp_config() | ||
| model_class = self._get_tp_model_class() | ||
| atol = self.tensor_parallel_atol | ||
| rtol = self.tensor_parallel_rtol | ||
|
|
@@ -555,7 +564,7 @@ def test_tp_forward(self): | |
| def test_tp_backward(self): | ||
| self._skip_if_not_supported() | ||
|
|
||
| config = self.model_tester.get_config() | ||
| config = self._get_tp_config() | ||
| model_class = self._get_tp_model_class() | ||
| atol = self.tensor_parallel_atol | ||
| rtol = self.tensor_parallel_rtol | ||
|
|
@@ -572,7 +581,7 @@ def test_tp_generation(self): | |
| # Test TP generation: unfused checkpoint → conversion mapping (if needed) → TP sharding → model → generate | ||
| self._skip_if_not_supported() | ||
|
|
||
| config = self.model_tester.get_config() | ||
| config = self._get_tp_config() | ||
|
|
||
| model_class = self._get_tp_model_class() | ||
| atol = self.tensor_parallel_atol | ||
|
|
@@ -594,7 +603,7 @@ def test_tp_generation_quantized(self): | |
| if not is_torchao_available(): | ||
| self.skipTest("Test requires torchao") | ||
|
|
||
| config = self.model_tester.get_config() | ||
| config = self._get_tp_config() | ||
| model_class = self._get_tp_model_class() | ||
| max_new_tokens = 25 | ||
|
|
||
|
|
@@ -611,7 +620,7 @@ def test_tp_generation_quantized(self): | |
| def test_ep_forward(self): | ||
| self._skip_if_not_supported(expert_parallel=True) | ||
|
|
||
| config = self.model_tester.get_config() | ||
| config = self._get_tp_config() | ||
| model_class = self._get_tp_model_class() | ||
| atol = self.tensor_parallel_atol | ||
| rtol = self.tensor_parallel_rtol | ||
|
|
@@ -627,7 +636,7 @@ def test_ep_forward(self): | |
| def test_ep_backward(self): | ||
| self._skip_if_not_supported(expert_parallel=True) | ||
|
|
||
| config = self.model_tester.get_config() | ||
| config = self._get_tp_config() | ||
| model_class = self._get_tp_model_class() | ||
| atol = self.tensor_parallel_atol | ||
| rtol = self.tensor_parallel_rtol | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since only this layer uses it, we can not make it generic for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it's a bit awkward. Wdy suggest? move it the the layer?