feat(skills): add xpk_to_clustertoolkit migration skill - #6183
feat(skills): add xpk_to_clustertoolkit migration skill#6183Neelabh94 wants to merge 20 commits into
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 introduces the xpk_to_clustertoolkit skill, enabling AI coding assistants and developers to seamlessly migrate legacy xpk CLI commands to Cluster Toolkit blueprints and submission commands. The changes include a deterministic translation parser, reference blueprints, and detailed documentation to streamline the infrastructure migration process. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an AI Skills Directory under skills/ to automate the migration of xpk CLI commands to Cluster Toolkit (gcluster) configurations. It includes a deterministic Python parser (parse_xpk_to_gcluster.py), reference blueprints, a migration guide, and associated tests. The reviewer identified several critical improvements for the Python parser: adding the missing machine_types.json reference file to prevent test failures, renaming the script-level --command flag to avoid conflicts with XPK arguments, using shlex.join to preserve shell quoting, and wrapping shlex.split in a try-except block to handle syntax errors gracefully.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new AI Skill directory to automate the migration of xpk CLI commands to Cluster Toolkit (gcluster) configurations, including a Python parser, unit tests, reference blueprints, and a migration guide. The review feedback highlights several important issues: a missing machine_types.json reference file that will cause test failures, a command-line parsing bug in the script's entry point when handling quoted arguments, a potential error when parsing zone variables without hyphens, a discrepancy in the documentation regarding the default storage mount mode, and the need to update script paths in SKILL.md to be relative to the repository root.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an AI skill for migrating XPK CLI configurations to Cluster Toolkit (gcluster). It includes a Python parser script to translate commands, reference blueprints for GKE A4 and TPU v7x, a migration guide, and unit/integration tests. The review feedback points out a syntax error in the TPU v7x blueprint where math operations are incorrectly combined inside a single expression block, and identifies edge cases in the Python parser where GPU-related bash variables starting with '$' could trigger incorrect TPU topology mapping or hardware detection.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the "XPK to Cluster Toolkit Migration Skill" under the skills/ directory. This includes a comprehensive migration guide, reference blueprints for TPU and GPU architectures, a deterministic Python parser (parse_xpk_to_gcluster.py) with unit tests, and helper scripts to automate the translation of xpk CLI commands to Cluster Toolkit (gcluster) commands and blueprints. Feedback on the changes highlights a critical missing reference file (machine_types.json) that will cause unit tests to fail, an invalid arithmetic expression in the Kueue configuration template variables within gke-tpu-7x.yaml, and a potential runtime crash in the Python parser when accessing unparsed flags.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the xpk_to_clustertoolkit AI skill, which automates the migration of xpk CLI commands to Cluster Toolkit (gcluster) equivalents. It includes a deterministic Python parser (parse_xpk_to_gcluster.py), reference blueprints, a migration guide, and associated tests. The review feedback highlights that several referenced files are missing from the PR, specifically machine_types.json, kueue-configuration.yaml.tftpl, and nccl-installer.yaml.tftpl. Additionally, suggestions were made to refactor the Python parser to initialize the warnings list earlier and to warn users when the --num-nodes flag is omitted for TPU jobs.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an AI Skills Directory to the Cluster Toolkit, specifically adding the xpk_to_clustertoolkit skill to automate the migration of xpk CLI commands to gcluster configurations. The changes include a deterministic Python parser, unit tests, blueprint templates, and a comprehensive migration guide. The review feedback highlights critical issues in the Python parser where the use of absl for flag parsing can cause crashes on unrecognized flags, suggesting a transition to standard library parsing. Additionally, a robustness improvement was suggested for the example-fetching script to gracefully handle network failures when offline.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an AI skill directory for migrating xpk CLI commands to Cluster Toolkit (gcluster) commands. It includes a deterministic Python parser script, shell helper scripts, reference blueprints, a migration guide, and unit tests. The review feedback suggests improving the Python parser to handle unquoted arguments gracefully and updating the shell script to use a dedicated hidden file for reliable cache freshness tracking.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new AI skill directory skills/xpk_to_clustertoolkit to automate the migration of xpk CLI commands to Cluster Toolkit (gcluster) configurations. This includes a deterministic Python parser script, unit tests, a detailed migration guide, reference blueprints, and pre-commit configuration updates. A review comment suggests improving the parsing of the --storage flag in the Python script to robustly handle bash variables and paths containing slashes when constructing mount paths.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'xpk_to_clustertoolkit' skill designed to assist AI coding assistants in migrating legacy XPK CLI commands to Cluster Toolkit (gcluster) blueprints and commands. The changes include a comprehensive migration guide, a deterministic Python parser for command translation, reference blueprint templates, and unit tests to ensure accuracy. The pre-commit configuration has been updated to exclude the new skills directory from markdown linting. I have no feedback to provide as the implementation is well-structured and follows the project's standards.
Neelabh94
added
the
release-new-modules
b70708a to
0a3e018
Compare
0a3e018 to
b0aed8d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Cluster Toolkit AI Skills Directory and the xpk_to_clustertoolkit skill, which automates the migration of legacy xpk CLI commands to Cluster Toolkit (gcluster) blueprints and commands. It includes a deterministic Python parser, unit tests, reference blueprints, and a migration guide. The review feedback is highly constructive, focusing on making the Python parser more robust when handling bash variables. Specifically, the reviewer suggests keeping generated blueprints generic by clearing deployment-specific variables and passing them dynamically via the --vars flag, dynamically deriving regions from zone variables, and omitting topology placeholders for variable TPU types to allow dynamic runtime resolution. All suggested changes are accompanied by corresponding unit test updates.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new AI Skills directory, specifically adding the xpk_to_clustertoolkit skill to automate the migration of legacy xpk CLI commands to Cluster Toolkit (gcluster) configurations. It includes comprehensive documentation, reference blueprints, evaluation tests, and a Python-based parser utility with unit tests. The review feedback is highly constructive and identifies a critical bug where the Lustre CSI driver flag is incorrectly mapped to the Parallelstore CSI driver, as well as an opportunity to refine machine type resolution for single-chip TPU topologies.
…om Parallelstore CSI driver
…les, and default mount mode
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new AI skill directory designed to automate the migration of legacy xpk CLI configurations and commands to Cluster Toolkit (gcluster). The changes include a deterministic Python parser, comprehensive unit tests, reference blueprints, and a detailed migration guide. The review feedback focuses on enhancing the Python parser by adding missing flag mappings (--timeout, --queue, --gke-namespace, and --skip-prereqs), improving command parsing robustness when environment variables are prepended, and explicitly specifying UTF-8 encoding when opening files.
…nd utf-8 file encoding
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an AI skill directory for migrating xpk CLI usages to Cluster Toolkit (gcluster) commands, complete with a deterministic Python parser, unit tests, reference blueprints, and a migration guide. The review feedback identifies a bug in the Python parser where environment variables in storage paths are not properly stripped from destination mount paths, and points out missing enable_managed_lustre_csi variable definitions and module settings in both the gke-tpu-7x.yaml and gke-a4.yaml reference blueprints.
| # Storage CSI Drivers | ||
| enable_gcsfuse_csi_driver: true | ||
| enable_filestore_csi_driver: false | ||
| enable_parallelstore_csi_driver: false | ||
| enable_pd_csi: false |
There was a problem hiding this comment.
The parser utility can output enable_managed_lustre_csi: true when the --enable-lustre-csi-driver flag is passed. However, the fallback blueprint template does not define this variable in its vars block, which will result in an unused variable warning or failure to enable the Lustre CSI driver when deploying the generated blueprint.
Let's add enable_managed_lustre_csi to the vars block.
# Storage CSI Drivers
enable_gcsfuse_csi_driver: true
enable_filestore_csi_driver: false
enable_parallelstore_csi_driver: false
enable_managed_lustre_csi: false
enable_pd_csi: false| enable_gcsfuse_csi: $(vars.enable_gcsfuse_csi_driver) | ||
| enable_filestore_csi: $(vars.enable_filestore_csi_driver) | ||
| enable_parallelstore_csi: $(vars.enable_parallelstore_csi_driver) | ||
| enable_pd_csi: $(vars.enable_pd_csi) |
There was a problem hiding this comment.
Let's also add the enable_managed_lustre_csi setting to the gke-tpu-7x-cluster module configuration so that the Lustre CSI driver is actually enabled on the GKE cluster when the variable is set to true.
enable_gcsfuse_csi: $(vars.enable_gcsfuse_csi_driver)
enable_filestore_csi: $(vars.enable_filestore_csi_driver)
enable_parallelstore_csi: $(vars.enable_parallelstore_csi_driver)
enable_managed_lustre_csi: $(vars.enable_managed_lustre_csi)
enable_pd_csi: $(vars.enable_pd_csi)References
- Explicitly define module settings in Cluster Toolkit blueprints, even if they are redundant with top-level global variables, to maintain parity with other blueprints and improve user experience (UX).
| # Storage CSI Drivers | ||
| enable_gcsfuse_csi_driver: true | ||
| enable_filestore_csi_driver: false | ||
| enable_parallelstore_csi_driver: false | ||
| enable_pd_csi: false |
There was a problem hiding this comment.
The parser utility can output enable_managed_lustre_csi: true when the --enable-lustre-csi-driver flag is passed. However, the fallback blueprint template does not define this variable in its vars block, which will result in an unused variable warning or failure to enable the Lustre CSI driver when deploying the generated blueprint.
Let's add enable_managed_lustre_csi to the vars block.
# Storage CSI Drivers
enable_gcsfuse_csi_driver: true
enable_filestore_csi_driver: false
enable_parallelstore_csi_driver: false
enable_managed_lustre_csi: false
enable_pd_csi: false| enable_gcsfuse_csi: $(vars.enable_gcsfuse_csi_driver) | ||
| enable_filestore_csi: $(vars.enable_filestore_csi_driver) | ||
| enable_parallelstore_csi: $(vars.enable_parallelstore_csi_driver) | ||
| enable_pd_csi: $(vars.enable_pd_csi) |
There was a problem hiding this comment.
Let's also add the enable_managed_lustre_csi setting to the a4-cluster module configuration so that the Lustre CSI driver is actually enabled on the GKE cluster when the variable is set to true.
enable_gcsfuse_csi: $(vars.enable_gcsfuse_csi_driver)
enable_filestore_csi: $(vars.enable_filestore_csi_driver)
enable_parallelstore_csi: $(vars.enable_parallelstore_csi_driver)
enable_managed_lustre_csi: $(vars.enable_managed_lustre_csi)
enable_pd_csi: $(vars.enable_pd_csi)References
- Explicitly define module settings in Cluster Toolkit blueprints, even if they are redundant with top-level global variables, to maintain parity with other blueprints and improve user experience (UX).
…p var chars in storage leaf paths
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new AI skills directory, specifically adding the xpk_to_clustertoolkit skill to automate the migration of legacy xpk CLI commands to Cluster Toolkit (gcluster) blueprints and workload commands. The changes include a deterministic Python parser, comprehensive unit tests, reference blueprints for GKE A4 and TPU 7x, and a detailed migration guide. The review comments correctly identify a bug in the 3D TPU topology mapping where chip counts are mapped to incorrect dimensions (e.g., 128 chips mapped to 4x4x4 instead of 4x4x8), and provide actionable suggestions to fix the mapping, the corresponding unit tests, and the documentation examples.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new AI Skills directory designed to automate the migration of legacy xpk CLI commands to Cluster Toolkit (gcluster) configurations. It includes a comprehensive migration guide, reference blueprints for GKE TPU and GPU architectures, and a deterministic Python parser utility (parse_xpk_to_gcluster.py) with accompanying unit tests. Feedback on the changes highlights a critical regex bug in the parser that misclassifies GPU types like v100 as TPU hardware (which would incorrectly omit the required --num-nodes flag), as well as an inconsistency in the documentation regarding whether the default storage mount mode is read-only (ro) or read-write (rw).
| d_lower = (device_type or "").lower() | ||
| if "tpu" in c_lower or "tpu" in d_lower: | ||
| return True | ||
| pattern = r"^(v\d|ct\d|tpu)" |
There was a problem hiding this comment.
The regex pattern ^(v\d|ct\d|tpu) matches any string starting with v followed by a digit. This causes a false positive for GPU types like v100 (matching v1), incorrectly classifying them as TPU hardware. As a result, the parser will omit the --num-nodes flag for GPU workloads, which is actually required. Suggest updating the pattern to target valid TPU versions (e.g., v2 through v7) to avoid matching v100.
| pattern = r"^(v\d|ct\d|tpu)" | |
| pattern = r"^(v[2-7]|ct[2-7]|tpu)" |
| - **Parallelstore**: Enable `enable_parallelstore_csi_driver: true` (or `enable_managed_lustre_csi: true` for Lustre) and configure `modules/file-storage/parallelstore`. | ||
| * **Job Submission Storage Mounting**: Use `--mount "<src>;<dest>[;<mode>][;options=<options>]"` with `gcluster job submit`. | ||
| - *Flag Translation*: Automatically converts XPK's `--storage <uri>` or `--storage <name>` flags into `--mount "<src>;<dest>;<mode>"`. | ||
| - *Default Mount Mode*: The `<mode>` field defaults to `ro` (read-only) if unspecified. |
There was a problem hiding this comment.
There is a documentation inconsistency regarding the default mount mode. SKILL.md states that the field defaults to ro (read-only) if unspecified, whereas xpk_to_clustertoolkit_migration_guide.md (line 212) and the parser implementation in parse_xpk_to_gcluster.py (line 275) default to rw (read-write). Please align the documentation to ensure consistency.
…in tpu hardware regex
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Cluster Toolkit AI Skills Directory, featuring the xpk_to_clustertoolkit skill. This skill is designed to help AI coding assistants automate the migration of legacy xpk CLI commands and configurations to declarative Cluster Toolkit (gcluster) blueprints and commands. The changes include the core skill definition, evaluation rubrics, a deterministic Python translation parser with comprehensive unit tests, reference blueprints for GKE A4 and TPU 7x, a detailed migration guide, and associated test assets. I have no feedback to provide as there are no review comments to evaluate.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the xpk_to_clustertoolkit AI skill directory, which automates the migration of legacy xpk CLI commands and configurations to the declarative Cluster Toolkit (gcluster) equivalent. The additions include a deterministic Python translation parser with unit tests, a comprehensive migration guide, and reference blueprints for GKE A4 and TPU v7x clusters. A critical issue was identified in the gke-tpu-7x.yaml reference blueprint, where an unconditionally enabled reservation_affinity block with an empty default reservation name will cause out-of-the-box Terraform deployment failures.
…d document in SKILL.md
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new AI skills directory, specifically adding the xpk_to_clustertoolkit skill to automate the migration of legacy xpk CLI commands to Cluster Toolkit (gcluster) configurations. The implementation includes a deterministic Python parser, unit tests, a comprehensive migration guide, and reference blueprints for TPU v7x and GPU A4 clusters. Feedback on the reference blueprints highlights that Multi-Tier Checkpointing (MTC) variables defined in the vars block are not mapped to the gke-tpu-7x-pool and a4-pool module settings, which would cause MTC configurations to be silently ignored during deployment.
…s and clean up unused mtc vars
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the xpk_to_clustertoolkit AI skill directory, which automates the migration of legacy xpk CLI commands and configurations to Cluster Toolkit (gcluster) equivalents. The changes include a Python-based parser utility, comprehensive unit tests, reference blueprints for GKE A4 and TPU v7x, a migration guide, and associated test files. One review comment was kept, which identifies a potential bash syntax error ('bad substitution') when parsing zone variables with parameter expansions and suggests a regex-based fix to ensure syntactical validity.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Cluster Toolkit AI Skills Directory, specifically adding the xpk_to_clustertoolkit skill. This skill automates the migration of legacy xpk CLI commands and configurations to modern, declarative Cluster Toolkit (gcluster) blueprints and workload submission commands. The changes include a deterministic Python parser (parse_xpk_to_gcluster.py) with comprehensive unit tests, reference blueprints for GKE A4, GKE TPU 7x, and storage architectures, a detailed migration guide, and evaluation tests. Additionally, the pre-commit configuration has been updated to exclude the skills directory from markdown linting. Since no review comments were provided, I have no feedback to offer on the review itself.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Cluster Toolkit AI Skills Directory and implements the xpk_to_clustertoolkit skill. This skill provides instructions, reference blueprints (for GKE A4, GKE TPU 7x, and storage), a migration guide, and a deterministic Python parser (parse_xpk_to_gcluster.py) with unit tests to automate the translation of legacy xpk CLI commands into native Cluster Toolkit (gcluster) configurations. I have no feedback to provide as there are no review comments and the changes are well-structured and thoroughly tested.
1 participant
Summary
This PR introduces the public
xpk_to_clustertoolkitskill to assist AI coding assistants and developers in migratingxpkCLI commands to Cluster Toolkit (gcluster) blueprints and submission commands.Key Changes
skills/xpk_to_clustertoolkit/SKILL.mddetailing 1:1 flag mappings, storage generation rules, Pathways submission flags, and Ray cluster configuration patterns.skills/xpk_to_clustertoolkit/scripts/parse_xpk_to_gcluster.pyproviding a deterministic Python parser for complexxpk workload createandxpk cluster createcommands.skills/xpk_to_clustertoolkit/scripts/parse_xpk_to_gcluster_test.py(15/15 passing).skills/README.mddocumenting the skill for public usage.Test Matrix
python3 -m unittest discover -s skills/xpk_to_clustertoolkit/scripts -p "*_test.py"(15/15 passed).addlicense,pytest-check,mypy-check,yamllint,shellcheck,shfmt,codespell(100% passed).