Skip to content

Update shell variable syntax in documentation to use ${VAR?} syntax#3306

Merged
copybara-service[bot] merged 1 commit intomainfrom
igorts/update_docs
Mar 7, 2026
Merged

Update shell variable syntax in documentation to use ${VAR?} syntax#3306
copybara-service[bot] merged 1 commit intomainfrom
igorts/update_docs

Conversation

@igorts-git
Copy link
Copy Markdown
Collaborator

@igorts-git igorts-git commented Mar 3, 2026

Description

This PR substitutes all code comments and example command lines in the documentation to use a less error-prone syntax.

When an environment variable is not set or misspelled, by default it is replaced with an empty string.
However, shell would fail to run a command if VAR is not set if we use the ${VAR?} syntax with the question mark.

This helps preventing common issues with using multiple terminal windows and avoids difficult-to-understand error messages.

This PR was vibe-coded with Gemini-CLI. I manually reviewed it and fixed some incorrect cases, such as ${USER_?}somestring, which is supposed to be ${USER}_somestring.

Tests

N/A. This PR only affects documentation.
I manually ran some commands to validate the syntax.

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.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@igorts-git igorts-git force-pushed the igorts/update_docs branch 3 times, most recently from b25f057 to 078185a Compare March 4, 2026 00:30
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 4, 2026

🤖 Hi @igorts-git, I've received your request, and I'm working on it now! You can track my progress in the logs for more details.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

## 📋 Review Summary

This PR consistently updates shell variable syntax in documentation and scripts to use ${VAR?} instead of $VAR to prevent silent failures when variables are unbound. The substitution improves the robustness of the example commands.

🔍 General Feedback

  • The changes successfully catch undefined environment variables in bash commands, producing a clear error message instead of failing silently.
  • One script (src/maxtext/examples/sft_train_and_evaluate.py) is missing a closing quote for a bash variable assignment, which will cause a critical syntax error if executed.
  • A documentation script example has an inline comment before a bash line continuation, breaking the execution block.
  • Overall, this is a solid robustness improvement for users copying scripts from the documentation.

Comment thread src/maxtext/examples/sft_train_and_evaluate.py Outdated
max_prefill_predict_length=$PREDICT_LENGTH # Adjust to fit image tokens + text prompt \
max_target_length=$TARGET_LENGTH \
max_prefill_predict_length=${PREDICT_LENGTH?} # Adjust to fit image tokens + text prompt \
max_target_length=${TARGET_LENGTH?} \
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 In bash, inline comments (`# ...`) take precedence over line continuations (`\`). This means the line continuation at the end of this line will be ignored, causing the command to break prematurely. You should move or remove the comment so the `\` works correctly.
Suggested change
max_target_length=${TARGET_LENGTH?} \
max_prefill_predict_length=${PREDICT_LENGTH?} \

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.

This is pseudo-code, I think it is fine to keep as-is. Otherwise it will be less readable.

@igorts-git igorts-git force-pushed the igorts/update_docs branch from 078185a to a9cf2c1 Compare March 4, 2026 01:27
Copy link
Copy Markdown
Collaborator

@richjames0 richjames0 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Copy Markdown
Collaborator

@khatwanimohit khatwanimohit left a comment

Choose a reason for hiding this comment

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

can we enforce a check to make sure future PRs also have this syntax.
Gemini suggested this,

#!/bin/bash
# check_env_syntax.sh

# Define excluded variables (Whiteslist)
EXCLUDES="PATH|HOME|PWD|SHELL|USER"

# Search for patterns like $VAR or ${VAR} that DON'T contain '?'
# This looks in .md and .sh files
ERRORS=$(grep -rnE '\$([A-Z_]+|\{[A-Z_]+\})' --include=\*.{md,sh} . | grep -vE "$EXCLUDES" | grep -v '?')

if [ -n "$ERRORS" ]; then
  echo "Error: Found raw env variables without the '?' syntax:"
  echo "$ERRORS"
  exit 1
fi

echo "Env variable syntax check passed."
exit 0

@copybara-service copybara-service Bot merged commit f690dc9 into main Mar 7, 2026
66 checks passed
@copybara-service copybara-service Bot deleted the igorts/update_docs branch March 7, 2026 00:27
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