diff --git a/.buildkite/matrix-test.rayci.yaml b/.buildkite/matrix-test.rayci.yaml new file mode 100644 index 00000000..9e959532 --- /dev/null +++ b/.buildkite/matrix-test.rayci.yaml @@ -0,0 +1,41 @@ +group: matrix selection test +tags: + - matrix-test +steps: + # Matrix step with 5 values - only "selected-1" and "selected-2" should be run + # when filtered (others are skipped) + - label: "Matrix {{matrix.variant}}" + key: matrix-step + command: | + echo "Running variant: {{matrix.variant}}" + if [ "{{matrix.variant}}" != "selected-1" ] && [ "{{matrix.variant}}" != "selected-2" ]; then + echo "ERROR: This variant should not have been spawned!" + echo "Only 'selected-1' and 'selected-2' variants should run when matrix-test tag is used." + exit 1 + fi + echo "Correct variant spawned." + depends_on: forge + matrix: + setup: + variant: + - "selected-1" + - "selected-2" + - "skipped-1" + - "skipped-2" + - "skipped-3" + + # This step depends on only the "selected" variants + - label: "Depends on selected only" + key: depends-on-selected + command: echo "This step depends on only the selected variants" + depends_on: + - key: matrix-step + matrix: + variant: ["selected-1", "selected-2"] + - forge + + - label: "Final validation" + tags: always + key: final-validation + command: echo "Final validation" + depends_on: depends-on-selected diff --git a/raycicmd/converter.go b/raycicmd/converter.go index a21aa697..690ec308 100644 --- a/raycicmd/converter.go +++ b/raycicmd/converter.go @@ -115,13 +115,144 @@ func stepTags(step map[string]any) []string { return nil } +// matrixExpansionContext tracks matrix steps during expansion for later +// dependency resolution. When a step depends on a matrix step (e.g., "ray-build"), +// we need to know what keys it expanded into (e.g., ["ray-build-python310", ...]). +type matrixExpansionContext struct { + stepKeyToConfig map[string]*matrixConfig + stepKeyToExpanded map[string][]string +} + +func expandMatrixInGroups(gs []*pipelineGroup) ([]*pipelineGroup, *matrixExpansionContext, error) { + ctx := &matrixExpansionContext{ + stepKeyToConfig: make(map[string]*matrixConfig), + stepKeyToExpanded: make(map[string][]string), + } + var result []*pipelineGroup + + for _, g := range gs { + newGroup := &pipelineGroup{ + filename: g.filename, + sortKey: g.sortKey, + Group: g.Group, + Key: g.Key, + Tags: g.Tags, + SortKey: g.SortKey, + DependsOn: g.DependsOn, + DefaultJobEnv: g.DefaultJobEnv, + } + + for _, step := range g.Steps { + matrixDef, hasMatrix := step["matrix"] + if !hasMatrix { + // No matrix, keep step as-is + newGroup.Steps = append(newGroup.Steps, step) + continue + } + + baseKey := stepKey(step) + if baseKey == "" { + // No key - pass through to Buildkite for native matrix handling + newGroup.Steps = append(newGroup.Steps, step) + continue + } + + // Parse matrix configuration + cfg, err := parseMatrixConfig(matrixDef) + if err != nil { + return nil, nil, fmt.Errorf("parse matrix in step %q: %w", stepKey(step), err) + } + + // Validate label has placeholder + if label, ok := step["label"].(string); ok { + if !hasMatrixPlaceholder(label) { + return nil, nil, fmt.Errorf("matrix step %q: label must contain {{matrix...}} placeholder", baseKey) + } + } + + // Register for selector expansion + ctx.stepKeyToConfig[baseKey] = cfg + + instances := cfg.expand() + if len(instances) == 0 { + return nil, nil, fmt.Errorf("matrix step %q: no instances after expansion", baseKey) + } + + var expandedKeysList []string + for _, inst := range instances { + expandedStep := inst.substituteValues(step).(map[string]any) + + expandedKey := inst.generateKey(baseKey, cfg) + if _, hasName := expandedStep["name"]; hasName { + expandedStep["name"] = expandedKey + } else { + expandedStep["key"] = expandedKey + } + delete(expandedStep, "matrix") + + originalTags := stepTags(step) + matrixTags := inst.generateTags() + allTags := append([]string{}, originalTags...) + allTags = append(allTags, matrixTags...) + if len(allTags) > 0 { + expandedStep["tags"] = allTags + } + + expandedKeysList = append(expandedKeysList, expandedKey) + newGroup.Steps = append(newGroup.Steps, expandedStep) + } + + ctx.stepKeyToExpanded[baseKey] = expandedKeysList + } + + result = append(result, newGroup) + } + + return result, ctx, nil +} + +// expandDependsOnSelectors processes depends_on to expand matrix selectors. +func expandDependsOnSelectors(dependsOn any, ctx *matrixExpansionContext) ([]string, error) { + selectors, err := parseMatrixDependsOn(dependsOn) + if err != nil { + return nil, err + } + + var result []string + for _, sel := range selectors { + if sel.Matrix == nil { + // Simple key reference - check if it's a matrix step + if expanded, ok := ctx.stepKeyToExpanded[sel.Key]; ok { + // Matrix step: expand to all expanded keys + result = append(result, expanded...) + } else { + // Non-matrix step: use key as-is + result = append(result, sel.Key) + } + } else { + matches, err := sel.expand(ctx.stepKeyToConfig) + if err != nil { + return nil, err + } + result = append(result, matches...) + } + } + + return result, nil +} + func (c *converter) convertGroups(gs []*pipelineGroup, filter *stepFilter) ( []*bkPipelineGroup, error, ) { + expandedGroups, matrixCtx, err := expandMatrixInGroups(gs) + if err != nil { + return nil, fmt.Errorf("expand matrix: %w", err) + } + set := newStepNodeSet() var groupNodes []*stepNode - for i, g := range gs { + for i, g := range expandedGroups { groupNode := &stepNode{ id: fmt.Sprintf("g%d", i), key: g.Key, @@ -169,8 +300,20 @@ func (c *converter) convertGroups(gs []*pipelineGroup, filter *stepFilter) ( for _, step := range groupNode.subSteps { // Track step dependencies. if dependsOn, ok := step.src["depends_on"]; ok { - deps := toStringList(dependsOn) - for _, dep := range deps { + // Expand matrix selectors in depends_on + expandedDeps, err := expandDependsOnSelectors(dependsOn, matrixCtx) + if err != nil { + return nil, fmt.Errorf("expand depends_on for step %q: %w", step.key, err) + } + + // Update the step source with expanded deps for Buildkite output + if len(expandedDeps) == 1 { + step.src["depends_on"] = expandedDeps[0] + } else { + step.src["depends_on"] = expandedDeps + } + + for _, dep := range expandedDeps { if depNode, ok := set.byKey(dep); ok { set.addDep(step.id, depNode.id) } diff --git a/raycicmd/converter_test.go b/raycicmd/converter_test.go index 69d7c5f4..1f87b23e 100644 --- a/raycicmd/converter_test.go +++ b/raycicmd/converter_test.go @@ -1,11 +1,11 @@ package raycicmd import ( - "testing" - "encoding/json" "fmt" "reflect" + "strings" + "testing" ) func TestParseStepEnvs(t *testing.T) { @@ -996,3 +996,288 @@ func TestConvertPipelineGroups(t *testing.T) { t.Errorf("convertPipelineGroup: got %d steps, want 2", len(bk1.Steps)) } } + +func TestConvertPipelineGroups_MatrixExpansion(t *testing.T) { + const buildID = "abc123" + info := &buildInfo{buildID: buildID} + + c := newConverter(&config{ + CITemp: "s3://ci-temp/", + RunnerQueues: map[string]string{"default": "runner"}, + }, info) + + groups := []*pipelineGroup{{ + Group: "build", + Steps: []map[string]any{{ + "label": "Build {{matrix.python}}", + "key": "build-step", + "commands": []any{"echo {{matrix.python}}"}, + "matrix": map[string]any{ + "setup": map[string]any{ + "python": []any{"3.10", "3.11"}, + }, + }, + }}, + }} + + filter := &stepFilter{runAll: true} + bk, err := c.convertGroups(groups, filter) + if err != nil { + t.Fatalf("convertGroups() error = %v", err) + } + + if len(bk) != 1 { + t.Fatalf("got %d groups, want 1", len(bk)) + } + + // Should have 2 expanded steps (no meta wait-step) + if len(bk[0].Steps) != 2 { + t.Fatalf("got %d steps, want 2", len(bk[0].Steps)) + } + + // Check expanded step keys (periods removed for valid Buildkite keys) + step0 := bk[0].Steps[0].(map[string]any) + if got := step0["key"]; got != "build-step-python310" { + t.Errorf("step[0].key = %q, want %q", got, "build-step-python310") + } + + step1 := bk[0].Steps[1].(map[string]any) + if got := step1["key"]; got != "build-step-python311" { + t.Errorf("step[1].key = %q, want %q", got, "build-step-python311") + } +} + +func TestConvertPipelineGroups_MatrixSelectorDependsOn(t *testing.T) { + const buildID = "abc123" + info := &buildInfo{buildID: buildID} + + c := newConverter(&config{ + CITemp: "s3://ci-temp/", + RunnerQueues: map[string]string{"default": "runner"}, + }, info) + + groups := []*pipelineGroup{{ + Group: "build", + Steps: []map[string]any{ + { + "label": "Build {{matrix.python}}", + "key": "build-step", + "commands": []any{"echo build"}, + "matrix": map[string]any{ + "setup": map[string]any{ + "python": []any{"3.10", "3.11"}, + }, + }, + }, + { + "label": "Test 3.11 only", + "key": "test-step", + "commands": []any{"echo test"}, + "depends_on": []any{ + map[string]any{ + "key": "build-step", + "matrix": map[string]any{ + "python": "3.11", + }, + }, + }, + }, + }, + }} + + filter := &stepFilter{runAll: true} + bk, err := c.convertGroups(groups, filter) + if err != nil { + t.Fatalf("convertGroups() error = %v", err) + } + + // Find the test step and check its depends_on + var testStep map[string]any + for _, s := range bk[0].Steps { + step := s.(map[string]any) + if step["key"] == "test-step" { + testStep = step + break + } + } + + if testStep == nil { + t.Fatal("test-step not found") + } + + // depends_on should be expanded to the specific python 3.11 key (periods removed) + dependsOn := testStep["depends_on"] + want := "build-step-python311" + if got, ok := dependsOn.(string); ok { + if got != want { + t.Errorf("depends_on = %q, want %q", got, want) + } + } else if gotSlice, ok := dependsOn.([]string); ok { + if len(gotSlice) != 1 || gotSlice[0] != want { + t.Errorf("depends_on = %v, want [%q]", gotSlice, want) + } + } else { + t.Errorf("depends_on unexpected type %T", dependsOn) + } +} + +func TestConvertPipelineGroups_MatrixTagFiltering(t *testing.T) { + const buildID = "abc123" + info := &buildInfo{buildID: buildID} + + c := newConverter(&config{ + CITemp: "s3://ci-temp/", + RunnerQueues: map[string]string{"default": "runner"}, + }, info) + + groups := []*pipelineGroup{{ + Group: "build", + Steps: []map[string]any{{ + "label": "Build {{matrix.python}}", + "key": "build-step", + "commands": []any{"echo build"}, + "matrix": map[string]any{ + "setup": map[string]any{ + "python": []any{"3.10", "3.11"}, + }, + }, + }}, + }} + + // Filter to only python-3.11 using auto-generated tag + filter := &stepFilter{tags: stringSet("python-3.11")} + bk, err := c.convertGroups(groups, filter) + if err != nil { + t.Fatalf("convertGroups() error = %v", err) + } + + if len(bk) != 1 { + t.Fatalf("got %d groups, want 1", len(bk)) + } + + // Should have only 1 expanded step (python-3.11) + meta wait-step = 2 steps + // Note: meta step may or may not be included depending on whether anything depends on it + // In this case with no dependents, it should be filtered out + if len(bk[0].Steps) != 1 { + t.Fatalf("got %d steps, want 1 (only python-3.11 step)", len(bk[0].Steps)) + } + + step0 := bk[0].Steps[0].(map[string]any) + if got := step0["key"]; got != "build-step-python311" { + t.Errorf("step[0].key = %q, want %q", got, "build-step-python311") + } +} + +func TestConvertPipelineGroups_MatrixLabelPlaceholderRequired(t *testing.T) { + const buildID = "abc123" + info := &buildInfo{buildID: buildID} + + c := newConverter(&config{ + CITemp: "s3://ci-temp/", + RunnerQueues: map[string]string{"default": "runner"}, + }, info) + + groups := []*pipelineGroup{{ + Group: "build", + Steps: []map[string]any{{ + "label": "Build step", // No {{matrix...}} placeholder + "key": "build-step", + "commands": []any{"echo build"}, + "matrix": map[string]any{ + "setup": map[string]any{ + "python": []any{"3.10", "3.11"}, + }, + }, + }}, + }} + + filter := &stepFilter{runAll: true} + _, err := c.convertGroups(groups, filter) + if err == nil { + t.Fatal("expected error for missing placeholder, got nil") + } + if !strings.Contains(err.Error(), "placeholder") { + t.Errorf("error = %q, want to contain \"placeholder\"", err.Error()) + } +} + +func TestConvertPipelineGroups_MatrixBaseKeyDependencyExpansion(t *testing.T) { + // Verify that when a downstream step depends on the base key, + // the dependency is expanded to all expanded keys directly. + const buildID = "abc123" + info := &buildInfo{buildID: buildID} + + c := newConverter(&config{ + CITemp: "s3://ci-temp/", + RunnerQueues: map[string]string{"default": "runner"}, + }, info) + + groups := []*pipelineGroup{{ + Group: "build", + Steps: []map[string]any{ + { + "label": "Build {{matrix.python}}", + "key": "build-step", + "commands": []any{"echo build"}, + "matrix": map[string]any{ + "setup": map[string]any{ + "python": []any{"3.10", "3.11"}, + }, + }, + }, + { + "label": "Final validation", + "key": "final-step", + "commands": []any{"echo final"}, + "tags": []any{"run-me"}, + "depends_on": "build-step", // Depends on base key + }, + }, + }} + + // Filter to only "run-me" tag - this selects final-step, + // which should pull in all expanded steps via dependency propagation + filter := &stepFilter{tags: stringSet("run-me")} + bk, err := c.convertGroups(groups, filter) + if err != nil { + t.Fatalf("convertGroups() error = %v", err) + } + + if len(bk) != 1 { + t.Fatalf("got %d groups, want 1", len(bk)) + } + + // Should have: 2 expanded steps + 1 final step = 3 steps (no meta-step) + if len(bk[0].Steps) != 3 { + var keys []string + for _, s := range bk[0].Steps { + step := s.(map[string]any) + if k, ok := step["key"]; ok { + keys = append(keys, k.(string)) + } + } + t.Fatalf("got %d steps %v, want 3 (2 expanded + final)", len(bk[0].Steps), keys) + } + + // Find final-step and verify its depends_on was expanded + var finalStep map[string]any + for _, s := range bk[0].Steps { + step := s.(map[string]any) + if step["key"] == "final-step" { + finalStep = step + break + } + } + if finalStep == nil { + t.Fatal("final-step not found") + } + + // depends_on should be expanded to both python versions + dependsOn, ok := finalStep["depends_on"].([]string) + if !ok { + t.Fatalf("depends_on is %T, want []string", finalStep["depends_on"]) + } + if len(dependsOn) != 2 { + t.Errorf("depends_on has %d elements, want 2: %v", len(dependsOn), dependsOn) + } +}