Layer checking as products pass over edges#704
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 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 👍 / 👎.
There was a problem hiding this comment.
@codex Can you suggest how I might detect this situation?
There was a problem hiding this comment.
Codex couldn't complete this request. Try again later.
| 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 👍 / 👎.
There was a problem hiding this comment.
@codex Strictly, those products are also distinguishable by stage name so a stage name should be provided. Our semantics are that where no stage name is provided that ambiguity is resolved by preferring the product in the current stage.
Nevertheless, can you suggest a way to detect this ambiguity where it actually exists?
There was a problem hiding this comment.
Codex couldn't complete this request. Try again later.
There was a problem hiding this comment.
@codex Partially resolved by bailing early if a stage name other than "CURRENT" is requested.
There was a problem hiding this comment.
Codex couldn't complete this request. Try again later.
knoepfel
left a comment
knoepfel
left a comment
There was a problem hiding this comment.
@beojan, thanks for the PR. I want to check a few things first with you:
- Multiple incoming edges to one input port are allowed so long as the source nodes of those incoming edges differ only in the layer.
- Assuming (1) is true, that means the only ports that can have multiple incoming edges are those input ports that are connected to provider nodes. All other input ports are connected to nodes that have differing names (where I assume a provider-node name cannot clash with a computational-node name)
- Assuming (2) is true, the only layer-checking that is necessary is the following situation: Node B wants to receive only layer
pdata from Node A, whereas Node A receives both layerpand layerqdata from corresponding upstream provider nodes.
Does that accord with your understanding?
| { | ||
| // create a product to be found | ||
| g.fold( | ||
| "duplicate_temperature", |
There was a problem hiding this comment.
This should trigger an exception before executing the graph: there's already a transform algorithm named "duplicate_temperature" earlier in the file.
There was a problem hiding this comment.
It certainly doesn't appear to.
| CHECK(g.execution_count("creator_and_layer_after_transform") == num_events); | ||
| } | ||
|
|
||
| SECTION("Products from this job, using layer only") |
There was a problem hiding this comment.
Does the section above (i.e., SECTION("Layer alone, distinguished by type")) not already cover this case?
There was a problem hiding this comment.
No, the section above specifies the layer only because layer is a mandatory field. The product lookup is done by type.
There was a problem hiding this comment.
But with your changes, wouldn't the lookup then also be done by layer as well?
I'm trying to determine how the construction you've added is different than what's already there...and it's not clear to me how it is.
There was a problem hiding this comment.
The selector requires layer to be event and type to be string (LIST char). Before and after my change, the product is uniquely identified by the type alone (i.e. there's only one node producing string products).
In the new test, the layer name is required to disambiguate, since there are two duplicate_temperature nodes producing temperature products of type double in the current stage.
|
1 is how I understand things, 2 evidently seems to be untrue. It appears nodes can share names so long as you don't try to wire them directly to each other. I'm less sure 1 should be true. If you have one path for generating and simulating MC and another for collecting real data, it doesn't seem to be that useful to say "Phlex can handle both in one stream and deal with the layer hierarchies differing, but only if creator names and product suffixes are identical in both paths". |
|
3 also doesn't follow from 2 and 1. If you write a selector that matches some product created by a previous node in every way except the layer is wrong, it shouldn't match anything. Without layer checking, it'll still match. |
This is a fair point, and I now realize the issue is more nuanced than I thought. If all of the product-selector fields need to be identical except for the layer, then that means that, for the same stage, the algorithm names need to be identical. And I argue that should be an error—nodes in the same stage are currently distinguishable (to the user) via the algorithm name, and thus they should be unique. So...that means we have a data-product provenance issue we need to disentangle. Will it be a problem for users if not only the data layers differ upon node execution, but also the originating provenance (creator names, stage names, etc.) of the input products? Maybe we can talk about this on Monday? |
…s a different stage
Clang-Tidy Check ResultsFound 5788 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
3 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