feat(wanda): Add dependency resolution for wanda specs

[andrew-anyscale]

Add buildDepGraph() to parse wanda specs and resolve dependencies by scanning the repo's spec_dirs for *.wanda.yaml files. The dependency graph is built in deterministic topological ordering.

Key features:

• Automatic discovery by scanning repo from .wandaspecs at git root
• Cycle detection with helpful error messages
• Variable expansion with unexpanded var detection

Design Decisions

1. Name collisions: Error on collision during index building. If two specs expand to the same name, this indicates a real problem that should be fixed. The error message will list both file paths.

2. Non-git repos: Use the spec file's directory as the search root. This is a safe default that still enables discovery for local testing without requiring a git repository.

Topic: wanda-deps

[andrew-anyscale]

Reviews in this chain:
└#347 feat(wanda): Add dependency resolution for wanda specs
　└#348 feat(wanda): Build dependencies in topological order
　　└#368 feat(wanda): Add 'params' field for env var validation and discovery

[andrew-anyscale]

#	head	base	diff	date	summary
0	d7807b50	4f2948f5	diff	Jan 6 9:33 AM	9 files changed, 676 insertions(+), 6 deletions(-)
1	470ff97d	4f2948f5	diff	Jan 6 9:41 AM	1 file changed, 8 insertions(+), 4 deletions(-)
2	6f660b11	4f2948f5	diff	Jan 6 9:45 AM	1 file changed, 35 insertions(+)
3	c9aea31c	4f2948f5	diff	Jan 6 9:49 AM	0 files changed
4	c1346cb8	8d6bd662	diff	Jan 9 17:19 PM	14 files changed, 614 insertions(+), 237 deletions(-)
5	988af315	8d6bd662	diff	Jan 9 17:31 PM	2 files changed, 7 insertions(+), 5 deletions(-)
6	388c9c17	8d6bd662	diff	Jan 12 14:09 PM	4 files changed, 60 insertions(+), 42 deletions(-)
7	b822440f	1f056c13	rebase	Jan 12 17:22 PM	0 files changed
8	587684f9	1f056c13	diff	Jan 12 17:41 PM	2 files changed, 16 insertions(+), 9 deletions(-)
9	48b77597	1f056c13	diff	Jan 12 17:49 PM	1 file changed, 5 insertions(+), 2 deletions(-)
10	3d8245b0	1f056c13	diff	Jan 12 18:07 PM	3 files changed, 12 insertions(+), 1 deletion(-)
11	698c6cb9	1f056c13	diff	Jan 12 18:15 PM	1 file changed, 1 insertion(+), 1 deletion(-)
12	e8a41e21	1f056c13	diff	Jan 12 18:24 PM	2 files changed, 5 insertions(+), 5 deletions(-)
13	853d062b	1f056c13	diff	Jan 12 18:25 PM	1 file changed, 1 insertion(+), 1 deletion(-)
14	4c062ac3	1f056c13	diff	Jan 12 18:37 PM	1 file changed, 1 insertion(+), 1 deletion(-)
15	d437c29b	1f056c13	diff	Jan 12 18:49 PM	1 file changed, 3 insertions(+), 3 deletions(-)
16	835bb4c3	ebed2021	diff	Jan 13 8:30 AM	11 files changed, 105 insertions(+), 167 deletions(-)
17	9bd8415d	ebed2021	diff	Jan 13 8:45 AM	1 file changed, 3 insertions(+), 2 deletions(-)
18	2f656729	ebed2021	diff	Jan 13 8:49 AM	1 file changed, 7 insertions(+), 1 deletion(-)
19	3e32233e	f547b3c4	diff	Jan 13 9:32 AM	1 file changed, 8 insertions(+), 1 deletion(-)
20	7bd90b70	f547b3c4	diff	Jan 13 11:43 AM	5 files changed, 66 insertions(+), 29 deletions(-)
21	8193927f	f547b3c4	diff	Jan 13 11:51 AM	3 files changed, 27 insertions(+), 25 deletions(-)
22	eb8860b9	f547b3c4	diff	Jan 13 11:56 AM	1 file changed, 7 insertions(+), 6 deletions(-)
23	7f955fda	f547b3c4	diff	Jan 13 16:28 PM	3 files changed, 41 insertions(+), 142 deletions(-)

[gemini-code-assist]

Summary of Changes

Hello @andrew-anyscale, I'm Gemini Code Assist¹! 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 core dependency resolution mechanism for Wanda specifications. The primary goal is to enable a more intuitive, Bazel-like user experience where wanda build spec.yaml can automatically identify and build all necessary prerequisite images. This is achieved by allowing specs to declare their dependencies, constructing a dependency graph, and performing a topological sort to determine the correct build order, complete with cycle detection and validation.

Highlights

• Dependency Graph Implementation: Introduced a new DepGraph structure and BuildDepGraph function to manage and resolve dependencies between Wanda specs, including topological sorting using Kahn's algorithm.
• Spec File Enhancements: Added a Deps field to the Spec struct, allowing users to declare paths to prerequisite Wanda spec files. The ParseSpecFile function has also been exported for broader use.
• Dependency Validation and Cycle Detection: Implemented robust validation for @ref dependencies, ensuring they are correctly provided within the graph, and added cycle detection to prevent infinite loops in dependency chains.
• Transitive Dependency Discovery: The dependency resolution logic now supports transitive discovery, meaning if A depends on B, and B depends on C, building A will automatically discover and include C.
• Environment Variable Handling: New utility functions (checkUnexpandedVars, findUnexpandedVars) were added to validate and report unexpanded environment variables within spec fields, improving error reporting.
• Comprehensive Testing: Added extensive unit tests for the new dependency resolution logic, covering scenarios like linear chains, diamond dependencies, cycle detection, missing dependencies, and variable expansion.

🧠 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces dependency resolution for wanda specs, a significant feature that adds core dependency graph functionality. The implementation includes a dependency graph with topological sorting (Kahn's algorithm), cycle detection, and support for transitive dependencies. The changes are well-structured and accompanied by a comprehensive set of tests. My review focuses on improving robustness by handling a potential error, increasing performance in the topological sort algorithm, and enhancing maintainability in the spec variable expansion logic.

[aslonnie]

this is double-declaring the dependency, both in froms and in deps. can we have a design (yet backwards-compatible), that can just reference another image based on the name?

think about bazel, when referencing another rule as deps, it does not need to specify which file to find it.

kind of out of scope, but we might also think about the possibility to:

• declare multiple images in one wanda definition file
• inline dockerfile contents, or inversely, inline wanda rules definition in the dockerfile.

[aslonnie]

hmm... was this missed before? is this a bug?

and stringsExpanVar probably should be stringsExpandVar

could you put these two small fixes in another PR?

and maybe add a unit test that can cover the bug..

[andrew-anyscale]

I cleaned up quite a bit of this logic in the subsequent PR #368. With that PR we have defined params, making it easier to declare what should be possible in froms and name

[andrew-anyscale]

Same for https://github.com/ray-project/rayci/pull/347/files#r2684497209

[andrew-anyscale]

Apologies for the bloodbath of updates here 😭 Ran into some issues of namespace conflict for Ray names in test. These should be resolved now, so this is good for review again 🙏

[aslonnie]

could you update the PR's description / title? this PR seems to be just a rename now.

[aslonnie]

oh, sorry, there are two large / hidden files.

[aslonnie]

instead of scanning all dirs, we should have have a config input and scan just given directories (and their sub dirs). for ray repo, this is:

docker/
ci/docker/
.buildkite/release-automation/

[andrew-anyscale]

Going with a project-level file .wandaspecs

[aslonnie]

a lot of the scanning time is likely spent on scanning bazel generated files. :)

[aslonnie]

maybe split the renaming as a leading PR, and the toposort algorithm things into another?

[andrew-anyscale]

Let me know what you think about the specDirs variable, versus coding a config at the root of the project (e.g. .wandaspecdirs)

EDIT: Making change here to use this. This feels like it will work well for our case!

[aslonnie]

nit: could you keep the line limit to 80 char?

also, wanda does not have a "repo" concept. the root is workDir.

for this particular file, maybe:

• use a regular file path, rather than a custom relative. otherwise, bash shell completion feels weird.
• leave the default value to "", and in description says that if this flag's value is empty, we look for .wandaspecs under work_dir.

[andrew-anyscale]

Thinking for the future of using wanda locally, the one complication with workDir is that it means you'd need to always run all wanda commands from the project root. E.g.

# works
 wanda ci/docker/base.build.wanda.yaml

cd ci/docker
# does not work
wanda base.build.wanda.yaml

Not for the scope of this PR, but it would be interesting to think on whether we'd want to enable this type of UX at some point

[aslonnie]

wow! lol..

can this be just a -> b -> c?

I guess this is claude code showing off its ascii art skills..

[gitar-bot]

Code Review 👍 Approved with suggestions 5 resolved / 7 findings

Well-structured dependency resolution implementation with comprehensive test coverage. Two previous minor findings remain unresolved but are reasonable design tradeoffs.

Suggestions 💡 2 suggestions

Bug: discoverSpecs silently skips parse errors in the index

📄 wanda/deps.go:352-358

When parseSpecFile fails in the discovery workers (lines 369-372), the error is silently swallowed and the file is marked as skipped. This could mask legitimate issues like malformed YAML in actual wanda specs.

While this behavior makes sense for tolerance during discovery (not all .wanda.yaml files may be valid for this environment), consider logging a debug/warning message when a parse error occurs so users can diagnose issues:

if err != nil {
    log.Printf("debug: skipping %s: %v", path, err)
    outCh <- discovered{skipped: true}
    continue
}

At minimum, document this intentional behavior in the function comment.

Edge Case: findUnexpandedVars doesn't handle ${VAR} brace syntax

📄 wanda/deps.go:282-312

The findUnexpandedVars() function only detects $VAR patterns but doesn't handle the ${VAR} brace syntax which is common in shell-style variable expansion. If expandVar() supports brace syntax (or if users expect it to), unexpanded ${VAR} references would silently pass validation.

Consider adding support for brace syntax:

if s[i] == '$' && i+1 < len(s) {
    if s[i+1] == '{' {
        // Handle ${VAR} syntax
        j := i + 2
        for j < len(s) && s[j] != '}' { j++ }
        if j < len(s) {
            vars = append(vars, s[i:j+1])
        }
        i = j
        continue
    }
    // ... existing $VAR handling
}

Impact: Specs using ${VAR} syntax might have unexpanded variables that slip through validation.

Resolved ✅ 5 resolved

Edge Case: localDeps doesn't strip image tags from dependency names

📄 wanda/deps.go:142-147
The localDeps() function strips the namePrefix but doesn't handle image tags (e.g., cr.ray.io/rayproject/base:v1.0 would produce base:v1.0 instead of base). When the wanda spec index is built, it uses spec names without tags. If a user references a dependency with a tag, the lookup will fail to find the matching spec.

Consider stripping the tag after the prefix:

depName := strings.TrimPrefix(from, namePrefix)
if idx := strings.Index(depName, ":"); idx != -1 {
    depName = depName[:idx]
}
deps = append(deps, depName)

Alternatively, document that tags should not be used when referencing wanda-built dependencies.

Edge Case: findRepoRoot may fail with .git file (worktrees/submodules)

📄 wanda/deps.go:316-330
The findRepoRoot() function only checks for .git as a directory (info.IsDir()), but in git worktrees and submodules, .git is a file containing a reference to the actual git directory. This means the function will fail to detect repo roots in these scenarios and fall back to startDir.

Consider using os.Stat(gitPath) without the IsDir() check, or explicitly handle both cases:

if _, err := os.Stat(gitPath); err == nil {
    return dir
}

Impact: Users working in git worktrees or submodules will experience unexpected behavior where discovery scans only from the spec's directory rather than the actual repo root.

Code Quality: validateDeps() function is defined but never called

📄 wanda/deps.go:215-234
The validateDeps() method on depGraph is defined but never invoked anywhere in the code. The validation logic it performs (checking that all @-referenced dependencies exist in the graph) appears to be implicitly handled by discoverAndLoad() which returns an error if a referenced spec is not found.

Either:

1. Remove the unused function to avoid dead code
2. Integrate it into buildDepGraph() as an additional validation step if the explicit validation is desired

Suggested fix: Remove the unused function or add a call to it in buildDepGraph() before returning.

Edge Case: findRepoRoot may loop indefinitely on empty path edge case

📄 wanda/deps.go:302-318
The findRepoRoot() function relies on filepath.Dir(dir) eventually returning the same value as dir to terminate. While this works correctly on most systems, the termination condition parent == dir assumes the path normalization behavior is consistent.

On Unix systems, filepath.Dir("/") returns /, so this works. On Windows, filepath.Dir("C:\\") returns C:\\. However, if startDir is somehow empty or malformed, the behavior may be unexpected.

This is a minor concern since startDir comes from filepath.Dir(absPath) which should always be valid, but adding a safety bound (e.g., max iterations) would make the code more defensive.

Suggested improvement (optional):

func findRepoRoot(startDir string) string {
    dir := startDir
    for i := 0; i < 1000; i++ { // safety bound
        gitPath := filepath.Join(dir, ".git")
        if info, err := os.Stat(gitPath); err == nil && info.IsDir() {
            return dir
        }
        parent := filepath.Dir(dir)
        if parent == dir {
            return startDir
        }
        dir = parent
    }
    return startDir
}

Bug: Race condition: walkErr accessed before WalkDir completes

📄 wanda/deps.go:374-401
In discoverSpecs(), the code closes pathsCh immediately after filepath.WalkDir() returns, and worker goroutines read from pathsCh. However, there's no synchronization ensuring all workers have finished processing before walkErr is checked and returned.

The issue is that when filepath.WalkDir() encounters an error and returns early, walkErr may be checked while workers are still processing paths from the channel. This could lead to incomplete results being returned without an error.

More critically, if WalkDir fails early with an error, the function returns that error, but the goroutine go func() { wg.Wait(); close(outCh) }() continues running in the background, which could cause issues if the caller tries to use the result.

Suggested fix: Move the walkErr check and return after draining outCh (which happens after workers finish via wg.Wait()):

// After the for range outCh loop completes...
if walkErr != nil {
    return nil, fmt.Errorf("walk %s: %w", searchRoot, walkErr)
}

What Works Well

Clean implementation of topological sorting with cycle detection. Good separation of concerns between discovery, loading, and sorting phases. Comprehensive test suite covering linear chains, diamond dependencies, cycles, and transitive discovery scenarios.

Options

Auto-apply is off Gitar will not commit updates to this branch.
Display: compact Hiding non-applicable rules.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs)}