-
Notifications
You must be signed in to change notification settings - Fork 15
Layer checking as products pass over edges #704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,14 +147,16 @@ namespace phlex::detail { | |
|
|
||
| for (auto const& query : node->input()) { | ||
| auto* receiver_port = collector ? collector : &node->port(query); | ||
| auto producer = producers.find_producer(query, node->name()); | ||
| if (not producer) { | ||
| auto producer_ports = producers.find_producers(query, node->name()); | ||
| if (producer_ports.empty()) { | ||
| // Is there a way to detect mis-specified product dependencies? | ||
| result[node_name].push_back({query, receiver_port}); | ||
| continue; | ||
| } | ||
|
|
||
| make_edge(*producer->output_port, *receiver_port); | ||
| for (auto* producer : producer_ports) { | ||
| make_edge(*producer->output_port, *receiver_port); | ||
|
Comment on lines
+157
to
+158
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a consumer has Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
| } | ||
| return result; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,27 @@ | ||
| #include "phlex/core/producer_catalog.hpp" | ||
| #include "phlex/utilities/bulleted_list.hpp" | ||
| #include "phlex/utilities/hashing.hpp" | ||
|
|
||
| #include "fmt/format.h" | ||
| #include "fmt/ranges.h" | ||
| #include "spdlog/spdlog.h" | ||
| #include <ranges> | ||
|
|
||
| namespace phlex::detail { | ||
| producer_catalog::named_output_port const* producer_catalog::find_producer( | ||
| std::vector<producer_catalog::named_output_port const*> producer_catalog::find_producers( | ||
| product_selector const& query, phlex::experimental::algorithm_name const& consumer_name) const | ||
| { | ||
| // Will need an update when we have a way to set the current stage name | ||
| if (query.stage.has_value() && query.stage.value() != "CURRENT"_idq) { | ||
| spdlog::debug( | ||
| "{} requires a stage other than the current one. Assuming it comes from a provider.", | ||
| query.to_string()); | ||
| return {}; | ||
| } | ||
| if (producers_.empty()) { | ||
| spdlog::debug("No producers found. Skipping and assuming {} comes from a provider.", | ||
| query.to_string()); | ||
| return nullptr; | ||
| return {}; | ||
| } | ||
| // Now the only way b == e is if we have a suffix and nothing creates matching products | ||
| auto [b, e] = query.suffix.has_value() ? producers_.equal_range(*query.suffix) | ||
|
|
@@ -22,9 +30,18 @@ namespace phlex::detail { | |
| spdlog::debug( | ||
| "Failed to find an algorithm that creates {} products. Assuming it comes from a provider", | ||
| query.suffix.value_or("*"_id)); | ||
| return nullptr; | ||
| return {}; | ||
| } | ||
| std::map<std::string, named_output_port const*> candidates; | ||
| std::vector<named_output_port const*> candidates; | ||
|
|
||
| // Don't really want to copy these fields so we'll use hashes and store a pointer to one copy | ||
| std::map<std::uint64_t, experimental::identifier const*> suffixes; | ||
| if (query.suffix.has_value()) { | ||
| suffixes.emplace(query.suffix->hash(), &*query.suffix); | ||
| } | ||
| std::map<std::uint64_t, experimental::algorithm_name const*> creators; | ||
| std::map<std::uint64_t, type_id const*> types; | ||
|
|
||
| for (auto const& [key, producer] : std::ranges::subrange{b, e}) { | ||
| // Prevent self-edges | ||
| if (producer.node == consumer_name) { | ||
|
|
@@ -59,7 +76,14 @@ namespace phlex::detail { | |
| query.type.exact_name(), | ||
| producer.type.exact_name()); | ||
| } | ||
| candidates.emplace(producer.node.to_string(), &producer); | ||
| candidates.push_back(&producer); | ||
| if (!query.suffix.has_value()) { | ||
| suffixes.emplace(key.hash(), &key); | ||
| } | ||
| creators.emplace( | ||
| phlex::detail::hash(producer.node.plugin().hash(), producer.node.algorithm().hash()), | ||
| &producer.node); | ||
| types.emplace(hash_value(producer.type), &producer.type); | ||
| } | ||
| } else { | ||
| spdlog::debug("Creator name mismatch between ({}) and {}", | ||
|
|
@@ -72,17 +96,41 @@ namespace phlex::detail { | |
| spdlog::debug( | ||
| "Cannot identify product matching the query {}. Assuming it comes from a provider.", | ||
| query.to_string()); | ||
| return nullptr; | ||
| } | ||
|
|
||
| if (candidates.size() > 1ull) { | ||
| std::string msg = fmt::format("More than one candidate matches the query {}: \n{}\n", | ||
| static auto const port_to_node = | ||
| [](named_output_port const* p) -> experimental::algorithm_name const& { return p->node; }; | ||
| static auto const deref_view = | ||
| std::views::transform([]<typename T>(T const* p) -> T const& { return *p; }); | ||
| std::string msg = fmt::format("More than one candidate matches the query {}: \n{}", | ||
| query.to_string(), | ||
| bulleted_list(std::views::keys(candidates), | ||
| bulleted_list(std::views::transform(candidates, port_to_node), | ||
| /*indent=*/1)); | ||
| throw std::runtime_error(msg); | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @codex Can you suggest how I might detect this situation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Codex couldn't complete this request. Try again later. |
||
| } | ||
| spdlog::error(msg); | ||
|
|
||
| if (suffixes.size() > 1) { | ||
| spdlog::error("Not permitted -- distinguishable by suffix {}", | ||
| suffixes | std::views::values | deref_view); | ||
| } | ||
| if (creators.size() > 1) { | ||
| spdlog::error("Not permitted -- distinguishable by creator {}", | ||
| creators | std::views::values | | ||
| std::views::transform( | ||
| [](experimental::algorithm_name const* p) { return p->to_string(); })); | ||
| } | ||
| if (types.size() > 1) { | ||
| spdlog::error("Not permitted -- distinguishable by type {}", | ||
| types | std::views::values | deref_view); | ||
| } | ||
| throw std::runtime_error("Multiple products in candidate set -- see errors"); | ||
| } | ||
|
|
||
| return candidates.begin()->second; | ||
| return candidates; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,4 +99,23 @@ TEST_CASE("Querying products in different ways", "[graph]") | |
| g.execute(); | ||
| CHECK(g.execution_count("creator_and_layer_after_transform") == num_events); | ||
| } | ||
|
|
||
| SECTION("Products from this job, using layer only") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the section above (i.e.,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the section above specifies the layer only because layer is a mandatory field. The product lookup is done by type.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The selector requires layer to be In the new test, the layer name is required to disambiguate, since there are two |
||
| { | ||
| // create a product to be found | ||
| g.fold( | ||
| "duplicate_temperature", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should trigger an exception before executing the graph: there's already a transform algorithm named
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It certainly doesn't appear to. |
||
| [](std::atomic<double>& summary, double temp) { summary += temp; }, | ||
| concurrency::unlimited, | ||
| "job") | ||
| .input_family(product_selector{.creator = "input", .layer = "event"}) | ||
| .output_product_suffixes("temperature"); | ||
|
|
||
| // Find it | ||
| g.transform("layer_only", [](double const& d) { return d; }) | ||
| .input_family(product_selector{.layer = "job"}) | ||
| .output_product_suffixes("job_temp"); | ||
| g.execute(); | ||
| CHECK(g.execution_count("layer_only") == 1); // 1 job | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, aneventtransform producing suffixnumberand ajobprovider for the same suffix/type, ajobselector 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex couldn't complete this request. Try again later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex Partially resolved by bailing early if a stage name other than "CURRENT" is requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex couldn't complete this request. Try again later.