feat(raycicmd): add matrix expansion types and selector resolution#378
feat(raycicmd): add matrix expansion types and selector resolution#378andrew-anyscale wants to merge 1 commit intomainfrom
Conversation
|
Summary of ChangesHello @andrew-anyscale, 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 a significant new feature to Rayci: the ability to expand matrix definitions into distinct Buildkite steps. This enhancement provides users with fine-grained control over their CI/CD pipelines, enabling them to define dependencies that target specific combinations within a matrix. The core logic, implemented in a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
7466596 to
853ec65
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for expanding matrix configurations in RayCI, allowing for more granular control over build steps. The implementation in raycicmd/matrix_expand.go is well-structured and covers parsing, expansion, key/tag generation, and dependency resolution. The accompanying tests in raycicmd/matrix_expand_test.go are comprehensive and cover a wide range of scenarios, including edge cases and error conditions.
My review includes a couple of suggestions to improve performance and code clarity in the matrix expansion and value substitution logic. Overall, this is a solid addition.
853ec65 to
7450d3e
Compare
b7b5455 to
b488745
Compare
bc2e07e to
570e6e7
Compare
raycicmd/matrix_expand.go
Outdated
| for i, inst := range instances { | ||
| if inst.matches(sel) { | ||
| matches = append(matches, allExpanded[i]) | ||
| } | ||
| } | ||
|
|
||
| if len(matches) == 0 { | ||
| return nil, fmt.Errorf("no matches for selector {key: %q, matrix: %v}", sel.Key, sel.Matrix) | ||
| } |
There was a problem hiding this comment.
Details
In the matrixSelector.expand method, the code assumes that instances (generated by cfg.expand()) and expandedKeys[sel.Key] have exactly the same ordering. However, expandedKeys is populated externally and there's no guarantee it follows the same iteration order.
The code at line 325-327 matches instances by index position:
for i, inst := range instances {
if inst.matches(sel) {
matches = append(matches, allExpanded[i])
}
}If expandedKeys["ray-build"] was built with a different ordering (e.g., using a different iteration over maps), the wrong keys would be matched.
Suggested fix: Instead of relying on index-based matching, regenerate the keys directly:
for _, inst := range instances {
if inst.matches(sel) {
matches = append(matches, inst.generateKey(sel.Key, cfg))
}
}This removes the dependency on external ordering and makes the code more robust.
Was this helpful? React with 👍 / 👎
570e6e7 to
268d356
Compare
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
24b799a to
cbb9612
Compare
Rayci will expand matrix into individual Buildkite steps, allowing for fine-grained depends_on targeting of specific matrix combinations.
## Matrix Formats Supported
Simple array (anonymous dimension):
matrix: ["3.10", "3.11", "3.12"]
Named dimensions:
matrix:
setup:
python: ["3.10", "3.11"]
cuda: ["12.1.1", "12.8.1"]
## Key Generation
Expanded keys follow deterministic format:
{base-key}-{dim1}{val1}-{dim2}{val2}
Examples:
ray-build + python=3.11, cuda=12.1.1 → ray-build-cuda1211-python311
ray-build + ["linux", "darwin"] → ray-build-linux, ray-build-darwin
## Selector Syntax for depends_on
Simple reference (waits for ALL instances via meta-step):
depends_on: ray-build
Selector syntax (waits for matching instances only):
depends_on:
- key: ray-build
matrix:
cuda: "12.1.1" # matches all python versions with cuda 12.1.1
Multi-value selector:
depends_on:
- key: ray-build
matrix:
python: ["3.10", "3.11"] # matches specific python versions
Topic: matrix-expand
Signed-off-by: andrew <andrew@anyscale.com>
cbb9612 to
4ce2802
Compare
Rayci will expand matrix into individual Buildkite steps, allowing for fine-grained depends_on targeting of specific matrix combinations.
Matrix Formats Supported
Simple array (anonymous dimension):
matrix: ["3.10", "3.11", "3.12"]
Named dimensions:
matrix:
setup:
python: ["3.10", "3.11"]
cuda: ["12.1.1", "12.8.1"]
Key Generation
Expanded keys follow deterministic format:
{base-key}-{dim1}{val1}-{dim2}{val2}
Examples:
ray-build + python=3.11, cuda=12.1.1 → ray-build-cuda1211-python311
ray-build + ["linux", "darwin"] → ray-build-linux, ray-build-darwin
Selector Syntax for depends_on
Simple reference (waits for ALL instances via meta-step):
depends_on: ray-build
Selector syntax (waits for matching instances only):
depends_on:
- key: ray-build
matrix:
cuda: "12.1.1" # matches all python versions with cuda 12.1.1
Multi-value selector:
depends_on:
- key: ray-build
matrix:
python: ["3.10", "3.11"] # matches specific python versions
Topic: matrix-expand
Signed-off-by: andrew andrew@anyscale.com