Split logical rule for eval from for train#4523
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1972c3b to
7e78b58
Compare
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
7e78b58 to
77bb651
Compare
|
🤖 Hi @RissyRan, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
📋 Review Summary
This pull request introduces the ability to split logical axis rules between training and evaluation phases, allowing specialized sharding strategies (such as CP/TP) specifically for evaluation under fractional batch sizes. The overall architecture is very well thought out, but several correctness bugs in the fallback validation, active rule detection, and evaluation input sharding should be addressed to ensure seamless operation.
🔍 General Feedback
- Input Sharding Consistency: Ensure that other data loaders or input processing utilities also correctly pick up the active
logical_axis_rules_for_evalduring evaluation to prevent layout mismatches. - Model-Level Rule Lookups: In addition to Qwen3, double check that other models (like DeepSeek or custom layer architectures) have been migrated to call
get_logical_axis_rules()dynamically rather than accessingself.config.logical_axis_rulesdirectly, to ensure they respect the eval-specific sharding rules. - Test Coverage: Consider adding a unit test validating that when
custom_mesh_and_rule_for_evalis left as default, it successfully falls back and matches the main training logical rules as expected.
| """ | ||
| if config.using_pipeline_parallelism or config.eval_interval > 1: | ||
| return None | ||
| return config.logical_axis_rules |
There was a problem hiding this comment.
🟠 Checking config.eval_interval > 1 is a brittle proxy for determining whether evaluation and training rule sets differ. If a user runs evaluation with eval_interval = 1, this check will evaluate to False and return config.logical_axis_rules (the training rules), overriding and ignoring the custom evaluation rules set in the active context manager during evaluation.
Comparing the rule lists directly or checking if evaluation rules are defined is much more robust and correct.
| return config.logical_axis_rules | |
| if config.using_pipeline_parallelism or ( | |
| config.logical_axis_rules_for_eval is not None | |
| and config.logical_axis_rules_for_eval != config.logical_axis_rules | |
| ): |
There was a problem hiding this comment.
I definitly agree that eval_interval > 1 is not the best way for selecting logical rules. I have tried using flax meta data to collect logical rules but it gets some unit test failures. I will be working on b/536927795 to deprecate this usage as a followup step.
There was a problem hiding this comment.
Are you trying to distinguish if eval is turned on? If so, wondering if eval_per_device_batch_size a better indicator. I see default is 0:
maxtext/src/maxtext/configs/base.yml
Line 696 in 2ce3866
In MLPerf 6.0 runs, we set eval_interval=1. Or maybe you could check if eval_interval as -1:
maxtext/src/maxtext/configs/base.yml
Line 986 in 2ce3866
There was a problem hiding this comment.
Thank you for the suggestion! Updated using eval_per_device_batch_size > 0 now.
There was a problem hiding this comment.
The logic in
maxtext/src/maxtext/configs/types.py
Line 2811 in bff4616
eval_interval != -1 instead.
| if 0 < eval_steps <= eval_step_count: | ||
| break | ||
| with jax.set_mesh(mesh), nn_partitioning.axis_rules(logical_axis_rules): | ||
| with jax.set_mesh(mesh), nn_partitioning.axis_rules(logical_axis_rules_for_eval): |
There was a problem hiding this comment.
🟠 During evaluation, the batch eval_batch is sharded on line 728 via sharding.get_input_data_sharding(config, mesh), which uses config.logical_axis_rules (the training rules) by default. Since the evaluation loop runs inside the context manager nn_partitioning.axis_rules(logical_axis_rules_for_eval), this mismatch will cause costly resharding/communication overhead or compilation failures.
To resolve this, update sharding.get_input_data_sharding to accept an optional rules=None parameter:
def get_input_data_sharding(config, mesh, rules=None):
if rules is None:
rules = config.logical_axis_rules
# ... Use rules instead of config.logical_axis_rulesAnd then pass logical_axis_rules_for_eval here:
eval_batch = jax.device_put(
eval_batch, sharding.get_input_data_sharding(config, mesh, rules=logical_axis_rules_for_eval)
)There was a problem hiding this comment.
Good point. Updated
There was a problem hiding this comment.
I feel like the Gemini PR review shows better quality now.
| # Set as true a different logical rule is used for eval step | ||
| custom_mesh_and_rule_for_eval: "" |
There was a problem hiding this comment.
🟢 The comment says # Set as true a different logical rule is used for eval step, but custom_mesh_and_rule_for_eval is a string identifier for custom rules (like "ep-as-cp") rather than a boolean. Clarify the comment to prevent user confusion.
| # Set as true a different logical rule is used for eval step | |
| custom_mesh_and_rule_for_eval: "" | |
| # Customized mesh and logical rules for evaluation (e.g. "ep-as-cp") | |
| custom_mesh_and_rule_for_eval: "" |
| # Handle eval custom mesh and rule | ||
| if self.custom_mesh_and_rule_for_eval is CustomRule.DEFAULT: | ||
| # Fallback to primary rule if eval is DEFAULT | ||
| self.custom_mesh_and_rule_for_eval = self.custom_mesh_and_rule | ||
| else: | ||
| eval_config = self._load_mesh_config_from_yaml(self.custom_mesh_and_rule_for_eval.value) | ||
| if "logical_axis_rules" in eval_config: | ||
| self.logical_axis_rules_for_eval = eval_config["logical_axis_rules"] |
There was a problem hiding this comment.
🔴 The fallback logic is incomplete when custom_mesh_and_rule_for_eval is DEFAULT. While self.custom_mesh_and_rule_for_eval is correctly updated to copy self.custom_mesh_and_rule, self.logical_axis_rules_for_eval is never updated and remains empty []. This will cause evaluation steps to use empty sharding rules instead of falling back to the training sharding rules, leading to compilation failures or incorrect device sharding.
| # Handle eval custom mesh and rule | |
| if self.custom_mesh_and_rule_for_eval is CustomRule.DEFAULT: | |
| # Fallback to primary rule if eval is DEFAULT | |
| self.custom_mesh_and_rule_for_eval = self.custom_mesh_and_rule | |
| else: | |
| eval_config = self._load_mesh_config_from_yaml(self.custom_mesh_and_rule_for_eval.value) | |
| if "logical_axis_rules" in eval_config: | |
| self.logical_axis_rules_for_eval = eval_config["logical_axis_rules"] | |
| # Handle eval custom mesh and rule | |
| if self.custom_mesh_and_rule_for_eval is CustomRule.DEFAULT: | |
| # Fallback to primary rule if eval is DEFAULT | |
| self.custom_mesh_and_rule_for_eval = self.custom_mesh_and_rule | |
| self.logical_axis_rules_for_eval = self.logical_axis_rules | |
| else: | |
| eval_config = self._load_mesh_config_from_yaml(self.custom_mesh_and_rule_for_eval.value) | |
| if "logical_axis_rules" in eval_config: | |
| self.logical_axis_rules_for_eval = eval_config["logical_axis_rules"] | |
| else: | |
| self.logical_axis_rules_for_eval = self.logical_axis_rules |
77bb651 to
bd59acc
Compare
RissyRan
left a comment
There was a problem hiding this comment.
Thanks for the change! A few minor comments for naming.
I didn't see you test feature for logical_axis_rules_for_eval. Do you forsee potential conflict when applying separately?
| config = immutable_data["config"] # for helpers | ||
| logical_axis_rules = immutable_data["logical_axis_rules"] | ||
| logical_axis_rules_train = immutable_data["logical_axis_rules_train"] | ||
| logical_axis_rules_for_eval = immutable_data["logical_axis_rules_for_eval"] |
There was a problem hiding this comment.
Shall we align the name from logical_axis_rules_for_eval to logical_axis_rules_eval?
There was a problem hiding this comment.
I updated the names to
logical_axis_rules_for_trainlogical_axis_rules_for_eval
for consistency. Good point!
| if 0 < eval_steps <= eval_step_count: | ||
| break | ||
| with jax.set_mesh(mesh), nn_partitioning.axis_rules(logical_axis_rules): | ||
| with jax.set_mesh(mesh), nn_partitioning.axis_rules(logical_axis_rules_for_eval): |
There was a problem hiding this comment.
I feel like the Gemini PR review shows better quality now.
| train_step_partial = functools.partial(train_step, model, config, state_mesh_shardings, params_shardings) | ||
| train_step = diloco.build_diloco_train_step(config, train_step_partial, mesh=mesh) | ||
| data_sharding = sharding.get_input_data_sharding(config, mesh) | ||
| train_data_sharding = sharding.get_input_data_sharding(config, mesh, rules=config.logical_axis_rules) |
There was a problem hiding this comment.
I see some misalignment with logical_axis_rules_train and logical_axis_rules. Do you think we could let them aligned? i.e. we could use logical_axis_rules for training stage if that makes things simple?
There was a problem hiding this comment.
Agree! Now they are
data_sharding_for_traindata_sharding_for_eval
| """ | ||
| if config.using_pipeline_parallelism or config.eval_interval > 1: | ||
| return None | ||
| return config.logical_axis_rules |
There was a problem hiding this comment.
Are you trying to distinguish if eval is turned on? If so, wondering if eval_per_device_batch_size a better indicator. I see default is 0:
maxtext/src/maxtext/configs/base.yml
Line 696 in 2ce3866
In MLPerf 6.0 runs, we set eval_interval=1. Or maybe you could check if eval_interval as -1:
maxtext/src/maxtext/configs/base.yml
Line 986 in 2ce3866
bd59acc to
94ca83b
Compare
94ca83b to
583b6a6
Compare
583b6a6 to
de63b4a
Compare
|
Regarding to question
I added an integration test protecting this feature. Thank you for bringing it up! |
Description
We might want different sharding logical rule for eval compared with train, since the global train batch size and eval batch size are different. This PR enables this feature by introducing two new flags:
custom_mesh_and_rule_for_evallogical_axis_rules_for_evalWhen
custom_mesh_and_rule_for_evalis specified (non-default value), an alternative logical rule is used for the eval pipeline. A typical use case would be using fractional batch size for eval, so that CP/TP like shardings are enabled (only for eval). Same axis is used as other shardings, e.g. FSDP/EP, for the train step.Currently, we still use the same mesh for both train and eval steps. A potential future optimization would be split the meshes for train and eval, which is in theory possible since train and eval functions are in different jit.It may required careful checks.
This PR only supports auto sharding. For explicit sharding, the logical rule split is way more complexed. I would defer the explicit sharding support to 2nd PR.
FIXES: b/535639231
Tests
Example 1: base rule for train, custom rule for eval
Run on v5p-8: we use FSDP+EP for train step, and EP-as-CP custom rule for eval,
log
xprof
Example 2: custom rule for train, another custom rule for eval
Run on v5p-8: we use EP-as-DP for train step, and EP-as-CP custom rule for eval,
log
xprof
Checklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.