-
Notifications
You must be signed in to change notification settings - Fork 454
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.
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @saumyamalik, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the evaluation environment setup by introducing a condition that disables WEKA evaluation when the target cluster is identified as 'augusta'. This change ensures that the evaluation process correctly adapts to specific cluster configurations, providing more accurate control over resource utilization based on the deployment environment. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
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 the evaluation script to disable WEKA mounts and force execution on the 'augusta' cluster when 'augusta' is part of the specified cluster list. The implementation is straightforward. I have one suggestion to make the cluster detection logic more robust to prevent potential false-positive matches with cluster names that might contain 'augusta' as a substring.
| # 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| if [[ "$MODEL_LOCATION" == gs://* || "$CLUSTER" == *augusta* ]]; then | |
| if [[ "$MODEL_LOCATION" == gs://* || "$CLUSTER" =~ \baugusta\b ]]; then |
There was a problem hiding this comment.
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
saurabh111233212
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we shouldn't use saumya's HF TOKEN, should probably pull the whoami using beaker CLI
Note
Updates eval script to turn off WEKA mounts when targeting augusta and to use a per-user HF_TOKEN secret derived from the Beaker username.
EVALUATE_ON_WEKA=falsewhen--clustercontainsaugusta(in addition togs://models).whoamifrombeaker account whoami.env-secret#2: HF_TOKEN=${whoami}_HF_TOKENin both WEKA and non-WEKA paths.Written by Cursor Bugbot for commit df49429. This will update automatically on new commits. Configure here.