Skip to content

set to augusta when specified #1138

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.

Sign up for GitHub

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

Open
saumyamalik wants to merge 3 commits into
base: main
Choose a base branch
from
Open

set to augusta when specified #1138

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6269755
set to augusta when specified
saumyamalik Nov 3, 2025
d3f2f65
change token to finegrained
saumyamalik Nov 3, 2025
df49429
change saumya's HF TOKEN to user's
saurabh111233212 Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions scripts/eval/oe-eval.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,10 @@ fi

# cluster/weka mount logic: default true (to use non-augusta)
# if model starts with gs://, set evaluate_on_weka to false.
# If specified CLUSTER contains augusta, set evaluate_on_weka to false
# All the logic is now handled internally, the flag is useless but keeping for backwards compatibility since people have scripts with it
EVALUATE_ON_WEKA="true"
if [[ "$MODEL_LOCATION" == gs://* ]]; then
if [[ "$MODEL_LOCATION" == gs://* || "$CLUSTER" == *augusta* ]]; then
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 3, 2025

Choose a reason for hiding this comment

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

high

The glob pattern *augusta* is a bit too broad and could lead to false positives. For example, a cluster named my-augusta-test would match, but a hypothetical cluster named notaugusta would also match, which is likely not the desired behavior. Using a regular expression with word boundaries (\b) would make this check more robust by matching augusta as a whole word.

Suggested change
if [[ "$MODEL_LOCATION" == gs://* || "$CLUSTER" == *augusta* ]]; then
if [[ "$MODEL_LOCATION" == gs://* || "$CLUSTER" =~ \baugusta\b ]]; then

Copy link
Contributor

@saurabh111233212 saurabh111233212 Nov 3, 2025

Choose a reason for hiding this comment

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

its ok gemini dw abt it

Copy link

@cursor cursor bot Nov 3, 2025

Choose a reason for hiding this comment

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

Bug: Overly Broad Cluster Match Overrides User Choice

The "$CLUSTER" == *augusta* condition is overly broad, matching any cluster name containing "augusta" as a substring. This incorrectly triggers the EVALUATE_ON_WEKA="false" path, which then hardcodes --cluster ai2/augusta. Consequently, user-specified augusta-related clusters are silently overridden, potentially causing jobs to run on an unintended cluster.

Fix in Cursor Fix in Web

Copy link
Contributor

@saurabh111233212 saurabh111233212 Nov 3, 2025

Choose a reason for hiding this comment

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

eh its fine

EVALUATE_ON_WEKA="false"
fi

Expand Down Expand Up @@ -352,9 +353,9 @@ for TASK in "${TASKS[@]}"; do
# NOTE: For gantry args here and below, random numbers like #42 are added to the env variables because they need to be unique names. The numbers are ignored.
# Build gantry args
if [ "$EVALUATE_ON_WEKA" == "true" ]; then
GANTRY_ARGS="{\"env-secret\": \"OPENAI_API_KEY=openai_api_key\", \"weka\": \"oe-adapt-default:/weka/oe-adapt-default\", \"weka#44\": \"oe-training-default:/weka/oe-training-default\", \"env-secret#2\":\"HF_TOKEN=HF_TOKEN\", \"env#132\":\"VLLM_ALLOW_LONG_MAX_MODEL_LEN=1\", \"env-secret#42\": \"AZURE_EVAL_API_KEY=azure_eval_api_key\"${MAX_TOKENS_ARG}}"
GANTRY_ARGS="{\"env-secret\": \"OPENAI_API_KEY=openai_api_key\", \"weka\": \"oe-adapt-default:/weka/oe-adapt-default\", \"weka#44\": \"oe-training-default:/weka/oe-training-default\", \"env-secret#2\":\"HF_TOKEN=saumyam_HF_TOKEN\", \"env#132\":\"VLLM_ALLOW_LONG_MAX_MODEL_LEN=1\", \"env-secret#42\": \"AZURE_EVAL_API_KEY=azure_eval_api_key\"${MAX_TOKENS_ARG}}"
else
GANTRY_ARGS="{\"env-secret\": \"OPENAI_API_KEY=openai_api_key\", \"env-secret#43\": \"AZURE_EVAL_API_KEY=azure_eval_api_key\", \"env\":\"VLLM_ALLOW_LONG_MAX_MODEL_LEN=1\", \"env-secret#2\":\"HF_TOKEN=HF_TOKEN\", \"mount\": \"/mnt/filestore_1:/filestore\", \"env#111\": \"HF_HOME=/filestore/.cache/huggingface\", \"env#112\": \"HF_DATASETS_CACHE=/filestore/.cache/huggingface\", \"env#113\": \"HF_HUB_CACHE=/filestore/.cache/hub\"${MAX_TOKENS_ARG}}"
GANTRY_ARGS="{\"env-secret\": \"OPENAI_API_KEY=openai_api_key\", \"env-secret#43\": \"AZURE_EVAL_API_KEY=azure_eval_api_key\", \"env\":\"VLLM_ALLOW_LONG_MAX_MODEL_LEN=1\", \"env-secret#2\":\"HF_TOKEN=saumyam_HF_TOKEN\", \"mount\": \"/mnt/filestore_1:/filestore\", \"env#111\": \"HF_HOME=/filestore/.cache/huggingface\", \"env#112\": \"HF_DATASETS_CACHE=/filestore/.cache/huggingface\", \"env#113\": \"HF_HUB_CACHE=/filestore/.cache/hub\"${MAX_TOKENS_ARG}}"
Copy link

@cursor cursor bot Nov 3, 2025

Choose a reason for hiding this comment

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

Bug: Secret name change breaks dynamic token usage

The HF_TOKEN secret name was accidentally changed to saumyam_HF_TOKEN in both GANTRY_ARGS paths. This hardcodes a specific user's token, which is inconsistent with our dynamic secret patterns and could lead to failures in shared environments.

Fix in Cursor Fix in Web

fi

if [ "$EVALUATE_ON_WEKA" == "true" ]; then
Expand Down