Layer checking as products pass over edges#704
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.{cpp,cc,cxx,h,hpp}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{hpp,cpp}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.hpp📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (12)
📝 WalkthroughWalkthroughDeclared nodes now expose their associated TBB flow graph. Producer resolution returns multiple matches with ambiguity checks, computational edges connect all matches, and consumer ports filter messages by layer. A product-selection test covers layer-only selection. ChangesProduct selection flow
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
21 fixed, 0 new since branch point (4e8e474) ✅ 21 CodeQL alerts resolved since the previous PR commit
✅ 21 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
Clang-Tidy Check ResultsFound 5788 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb71620bf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (auto* producer : producer_ports) { | ||
| make_edge(*producer->output_port, *receiver_port); |
There was a problem hiding this comment.
When a consumer has experimental_when, receiver_port is the filter's collector, so this loop connects every matching producer directly to the filter and never calls node->port(query). The new layer-checking node is only created inside products_consumer::port(), while filter forwards to consumer.ports() (the raw downstream ports), so a filtered node whose selector asks for job can still receive matching event producer messages whenever the predicate accepts those message ids.
Useful? React with 👍 / 👎.
| if (suffixes.size() == 1 && creators.size() == 1 && types.size() == 1) { | ||
| spdlog::info(msg); | ||
| spdlog::info("This is permitted -- layers may differ"); | ||
| return candidates; |
There was a problem hiding this comment.
This branch permits multiple producers solely because suffix, creator, and type are identical, but the catalog has not checked that the candidates actually differ by layer. In configurations like a transform and a fold registered with the same name and suffix that both emit on the selector's layer, all candidates are returned and connected; the downstream layer checker accepts both, causing duplicate executions instead of the previous ambiguity error.
Useful? React with 👍 / 👎.
| if (producer_ports.empty()) { | ||
| // Is there a way to detect mis-specified product dependencies? | ||
| result[node_name].push_back({query, receiver_port}); |
There was a problem hiding this comment.
Because find_producers() is not layer-aware, a producer on a different layer still makes this branch treat the input as satisfied. In a graph with, for example, an event transform producing suffix number and a job provider for the same suffix/type, a job selector reaches this path, the provider lookup is skipped, and the new layer checker drops the event messages, leaving the consumer with no input instead of using the matching provider or reporting the product as unresolved.
Useful? React with 👍 / 👎.
2 participants
As discussed, layer checking happens dynamically as products pass over edges. A product selector without a layer specification would (in theory) accept products from any layer and place outputs appropriately so the layer a product belongs to need not be constant between iterations.
Layer specifications remain mandatory in selectors for now. Ironically until now they weren't actually being checked at all except when matching to products from providers.
Code
fold,observer,predicate,transform,unfold) by storing the owningtbb::flow::graphand exposing it viagraph() const.products_consumer, so product layer validation now happens as products traverse edges rather than only during producer matching.Tests
Behavior