Skip to content

pre-populate MODEL and remove --zone from multi-host RL tutorial#3986

Merged
copybara-service[bot] merged 1 commit into
mainfrom
doc_change_multihost
May 28, 2026
Merged

pre-populate MODEL and remove --zone from multi-host RL tutorial#3986
copybara-service[bot] merged 1 commit into
mainfrom
doc_change_multihost

Conversation

@Shuwen-Fang
Copy link
Copy Markdown
Collaborator

@Shuwen-Fang Shuwen-Fang commented May 26, 2026

Pre-populate MODEL with the tutorial's specific model name so users don't need to hunt through types.py. Remove --zone from xpk commands, consistent with the existing note that XPK v0.14+ auto-discovers cluster location.

Tests

N/A

Checklist

Before submitting this PR, please make sure (put X in square brackets):

  • I have performed a self-review of my code. For an optional AI review, add the gemini-review label.
  • I have necessary comments in my code, particularly in hard-to-understand areas.
  • I have run end-to-end tests tests and provided workload links above if applicable.
  • I have made or will make corresponding changes to the doc if needed, including adding new documentation pages to the relevant Table of Contents (toctree directive) as explained in our documentation.

@google-cla
Copy link
Copy Markdown

google-cla Bot commented May 26, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

# The MaxText model name. See `src/maxtext/configs/types.py` for `ModelName` for a
# full list of supported models.
export MODEL=<MODEL_NAME> # e.g. 'llama3.1-70b-Instruct'
export MODEL=llama3.1-70b-Instruct # replace with another model from src/maxtext/configs/types.py if needed
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It might be better if this was left as is? The existing state forces the user to actively set a model (which errors out when they forget to do so). Pre-setting a model might cause them to forget to update this causing a mismatch with what they might be expecting. This is a fillable box anyway on the documentation.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+1 please leave the editable variable name as is

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Collaborator

@bvandermoon bvandermoon left a comment

Choose a reason for hiding this comment

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

LGTM outside of the <MODEL_NAME> comment from @niting

# The MaxText model name. See `src/maxtext/configs/types.py` for `ModelName` for a
# full list of supported models.
export MODEL=<MODEL_NAME> # e.g. 'llama3.1-70b-Instruct'
export MODEL=llama3.1-70b-Instruct # replace with another model from src/maxtext/configs/types.py if needed
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+1 please leave the editable variable name as is

@Shuwen-Fang Shuwen-Fang force-pushed the doc_change_multihost branch from b5e5f90 to 7c976c2 Compare May 27, 2026 18:48
@Shuwen-Fang Shuwen-Fang requested a review from niting May 27, 2026 18:49
@copybara-service copybara-service Bot merged commit 03a192b into main May 28, 2026
32 checks passed
@copybara-service copybara-service Bot deleted the doc_change_multihost branch May 28, 2026 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants