Skip to content

fix(e2e): Fix dataset path for multimodal SFT, RL parallelism, and LLaMA to_hf settings - #4941

Open
RexBearIU wants to merge 13 commits into
mainfrom
jackyf/fix-e2e-post-training
Open

fix(e2e): Fix dataset path for multimodal SFT, RL parallelism, and LLaMA to_hf settings#4941
RexBearIU wants to merge 13 commits into
mainfrom
jackyf/fix-e2e-post-training

Conversation

@RexBearIU

Copy link
Copy Markdown
Collaborator

Description

Fixes end-to-end TPU post-training test suite configurations:

  1. Gemma-3-4B Multimodal SFT (tests/end_to_end/tpu/gemma3/4b/test_gemma3_multimodal_sft.sh):

    • Added export DATASET_PATH=${DATASET_PATH:-gs://maxtext-dataset} to point directly to GCS ChartQA parquet shards matching test_qwen3_multimodal_sft.sh.
  2. Gemma-4-26B RL (tests/end_to_end/tpu/gemma4/26b/test_gemma4_rl.sh):

    • Updated vLLM decode parallelism to ici_tensor_parallelism=4 ici_expert_parallelism=2 and hbm_utilization_vllm=0.6 for MoE sharding.
    • Updated GRPO RL training to train_micro_batch_size=4 rollout_micro_batch_size=8 ici_expert_parallelism=8 rollout_tensor_parallelism=4 to fit the 8 trainer / 8 sampler device partition on v5p-32.
    • Added vllm_additional_config to pass model name during Step 3 checkpoint decode.
  3. LLaMA-3.1-70B to HuggingFace (tests/end_to_end/tpu/llama3.1/70b/test_llama3.1_70b_to_hf.sh):

    • Set checkpoint_storage_concurrent_gb=80 and --parallel_threads=1 to optimize host RAM overhead during shard writing.

Tests

  • Verified test_gemma3_multimodal_sft.sh on TPU v5p-8.
  • Verified test_gemma4_rl.sh on TPU v5p-32.
  • Verified test_llama3.1_70b_to_hf.sh SafeTensors unrolling on TPU v5p-8.

Checklist

  • Pre-commit / linter checks pass (codespell, pylint, pyink).
  • All test scripts execute cleanly with standard parameters.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates several end-to-end TPU test scripts (Gemma 3, Gemma 4, and LLaMA 3.1) to adjust configurations, parallelism settings, batch sizes, and cleanup steps. Key feedback includes a recommendation to guard a recursive gcloud storage rm command in test_gemma3_multimodal_sft.sh to prevent accidental deletion of parent directories if ${run_id} is empty, and the removal of an unused vllm_additional_config parameter in test_gemma4_rl.sh.

skip_jax_distributed_system=True

# Step 3: Run SFT on the MaxText checkpoint on ChartQA dataset
gcloud storage rm --recursive ${BASE_OUTPUT_DIRECTORY}/multimodal/sft/${run_id} || true

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Running gcloud storage rm --recursive on a path that could potentially have an empty ${run_id} is dangerous, as it could result in the accidental deletion of the entire parent directory (${BASE_OUTPUT_DIRECTORY}/multimodal/sft/). It is highly recommended to guard this command with a check ensuring ${run_id} is non-empty.

Suggested change
gcloud storage rm --recursive ${BASE_OUTPUT_DIRECTORY}/multimodal/sft/${run_id} || true
if [ -n "${run_id}" ]; then gcloud storage rm --recursive ${BASE_OUTPUT_DIRECTORY}/multimodal/sft/${run_id} || true; fi

Comment on lines +55 to +56
vllm_hf_overrides='{architectures: ["MaxTextForCausalLM"]}' \
hbm_utilization_vllm=0.85 \
vllm_additional_config='{"maxtext_config": {"model_name": "gemma4-26b", "log_config": "false", "prefuse_moe_weights": "true"}}' \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The vllm_additional_config parameter is passed to vllm_decode.py here, but vllm_decode.py does not actually parse or use config.vllm_additional_config in either decode_with_vllm or decode_with_tunix. Passing this parameter is a no-op and should be removed to avoid confusion.

Suggested change
vllm_hf_overrides='{architectures: ["MaxTextForCausalLM"]}' \
hbm_utilization_vllm=0.85 \
vllm_additional_config='{"maxtext_config": {"model_name": "gemma4-26b", "log_config": "false", "prefuse_moe_weights": "true"}}' \
vllm_hf_overrides='{architectures: ["MaxTextForCausalLM"]}' \

@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@RexBearIU
RexBearIU force-pushed the jackyf/fix-e2e-post-training branch from 7b4b03b to 437f71d Compare August 19, 2026 08:09
@RexBearIU
RexBearIU requested a review from parambole as a code owner August 19, 2026 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant