fix(slurm/scripts): use locationPolicy.zones for AI zone support in Regional BulkInsert#5378
Conversation
Summary of ChangesHello, 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 modifies the resume.py script to correctly handle AI zones during regional bulk insert operations. The previous implementation was incompatible with AI zones due to its reliance on a dictionary-based location mapping. This change refactors the code to use an array-based zone specification, ensuring proper support for AI zones while maintaining existing functionality for zonal bulk inserts. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the regional BulkInsert payload to use locationPolicy.zones[] instead of locationPolicy.locations{}, which enables support for AI zones. The change is clear and directly addresses the issue described.
My review includes one suggestion to add a warning log when zone_policy_deny is configured for a regional bulk insert, as this functionality is being removed. This will help prevent unexpected behavior for users who might have this setting in their configurations.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly refactors the regional BulkInsert request payload to use locationPolicy.zones instead of locationPolicy.locations. This change enables support for AI-specific zones, which was the goal. The implementation is clean and removes the now-unused zone_deny logic, simplifying the code. I have one minor suggestion to improve the clarity of a comment.
There was a problem hiding this comment.
Code Review
This pull request correctly updates the regional BulkInsert payload to use locationPolicy.zones, which enables support for AI zones as intended. The code is now simpler and more aligned with the current Google Cloud API.
I've identified one potential issue where an empty zone_allow list could cause the API request to fail. I've added a specific comment with a suggestion to address this edge case. Otherwise, the change is excellent. I've adjusted the severity to medium.
c8402b6 to
dc97bec
Compare
abe15de
into
GoogleCloudPlatform:develop
Description
This PR updates the payload mapping for Regional
BulkInsertrequests in resume.py to support building instances inside AI zones.The Problem
The previous implementation used
locationPolicy.locations{}(a dictionary map). Compute Engine specifications do not support mapping AI-specific zones to that dictionary structure, causing AI zones to be either rejected (historical validation error) or silently ignored by the backend.The Solution
Refactored
create_instances_requestto specify explicitly target layouts using the arraylocationPolicy.zones[]structure:Testing Performed
Verified on a live Slurm Controller node utilizing simulated injection targetting multiple zones (confirming clean creation loops without crashes on standard regional nodes) and also verified that Zonal BulkInsert (len == 1) is modularly untouched.