[raycicmd] Implicit dimension matching for array-to-array depends_on#471
Conversation
|
Reviews in this chain: |
|
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 significantly enhances the 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces implicit dimension matching for array-to-array dependencies, a valuable feature for improving pipeline parallelism. The implementation is well-structured, adding logic for implicit filters while also providing a clear opt-out mechanism. The changes are supported by a comprehensive and robust test suite that covers a wide range of scenarios and edge cases. The code quality is high, and I have one suggestion to enhance documentation for better long-term maintainability.
| key string // step identifier: "key" for command steps, "name" for wanda steps | ||
| filter map[string][]string // dimension -> allowed values (nil = all instances) | ||
| key string // step identifier | ||
| filter map[string][]string // nil = implicit matching may apply |
There was a problem hiding this comment.
The comment for the filter field is a bit brief given its nuanced behavior. When filter is nil, it signifies a plain dependency, which can either trigger implicit dimension matching or result in a fan-out to all variants if implicit matching doesn't apply. Clarifying this behavior in the comment will improve maintainability for anyone working on this code in the future.
| filter map[string][]string // nil = implicit matching may apply | |
| filter map[string][]string // dimension constraints; nil for a plain dependency, which may trigger implicit matching or fan-out to all variants |
56bfbd0 to
03cd98f
Compare
|
Added |
03cd98f to
7efcbc4
Compare
b2ec364 to
2ca9c57
Compare
7efcbc4 to
ee66d67
Compare
ee66d67 to
bd4e5bd
Compare
3b3be0c to
66c3763
Compare
bd4e5bd to
339ff12
Compare
66c3763 to
91b2dcd
Compare
339ff12 to
c0fdd3e
Compare
91b2dcd to
f2f670d
Compare
c0fdd3e to
551d7bb
Compare
f2f670d to
fb306e4
Compare
c47615a to
5be334f
Compare
fb306e4 to
28bf469
Compare
aslonnie
left a comment
There was a problem hiding this comment.
PR description looks weird.
why skip-prcheck label is added for this PR?
5be334f to
b474b41
Compare
28bf469 to
5528dfc
Compare
|
added |
Implement ($) resolution: when an array step depends on another array step via ($), each expanded variant depends only on the target variant with matching values on shared dimensions. Guards: - ($) from a non-array step errors (must use (*)) - ($) with no overlapping dimensions errors (must use (*)) - Value mismatch in implicit matching produces a clear error Topic: implicit-dim-matching Relative: bracket-depends-on Signed-off-by: andrew <andrew@anyscale.com>
b474b41 to
3debefd
Compare
Implement ($) resolution: when an array step depends on another array step via ($ ), each expanded variant depends only on the target variant with matching values on shared dimensions.
Guards:
Topic: implicit-dim-matching
Relative: bracket-depends-on
Signed-off-by: andrew andrew@anyscale.com