Default Kueue config for Pathways#5628
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 streamlines the configuration management for Pathways by replacing complex, inline Terraform logic with a clean, template-based approach. It also introduces support for dynamic slicing and ensures that the underlying Helm controllers are correctly provisioned with appropriate resource defaults when these advanced features are enabled. 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 the 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 counterproductive. 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 refactors the Kueue configuration logic within the kubectl-apply module to support dynamic slicing and Pathways, introducing new templates and conditional resource allocation for controllers. Several critical issues were identified: the kueue variable should use type = any per style guidelines to handle complex structures; two referenced template files for dynamic slicing are missing from the PR; the accelerator_type variable is missing from the default template variables; the default resource requests for Kueue and JobSet controllers are potentially too high for standard nodes; and the apiVersion in the new template should be updated to v1beta2 for better compatibility.
b35030b to
3b24ea4
Compare
|
/gemini summary |
Summary of ChangesThis pull request simplifies configuration management for Pathways by replacing complex, inline Terraform logic with a robust, template-based system. It introduces dedicated template files for Pathways and dynamic slicing, refactors the main configuration logic to conditionally load these templates, and updates Helm controller resource defaults to better support these advanced features. These changes improve code maintainability and provide a more flexible configuration interface for users. Highlights
Activity
|
3b24ea4 to
2943f69
Compare
|
/gcbrun |
1 similar comment
|
/gcbrun |
SwarnaBharathiMantena
left a comment
There was a problem hiding this comment.
LGTM!
The failing tests are either capacity issues or known issues the team is working on.
40f1676 to
c37bfab
Compare
|
/gcbrun |
|
/gcbrun |
Neelabh94
left a comment
There was a problem hiding this comment.
Please address the comments above.
d7f54a4 to
d71a859
Compare
de5d7e3 to
f7f292b
Compare
6cb5c96 to
d11f84a
Compare
d11f84a to
82e9358
Compare
Done. I also added |
|
/gcbrun |
|
@SwarnaBharathiMantena can we get this merged? |
962b1d3
into
GoogleCloudPlatform:develop
Objective
Simplify the configuration management for Pathways and remove brittle inline YAML-merging logic.
Changes
main.tf.kueue/kueue-configuration-pathways.yaml.tftpl).main.tfto conditionally load this template whenenable_pathways_for_tpusis true and no customconfig_pathis provided.This cleanup makes the module logic much easier to maintain and establishes a clean pattern for adding default configurations for other advanced features.