Allow drivers to access configured sources#658
Open
knoepfel wants to merge 9 commits into
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthrough
ChangesDeferred Driver Configuration
Sequence DiagramsequenceDiagram
participant run as phlex::run()
participant load_driver as load_driver()
participant framework_graph as framework_graph
participant driver_proxy as driver_proxy
participant layer_generator as layer_generator
run->>framework_graph: with_deferred_driver(max_parallelism)
framework_graph-->>run: g (driver_ empty, mode=deferred)
run->>load_driver: load_driver(g, driver_config)
load_driver->>framework_graph: g.driver_proxy(required_sources)
framework_graph->>driver_proxy: driver_proxy(sources_for(required_sources))
driver_proxy-->>load_driver: d
load_driver->>driver_proxy: d.driver(generator) or d.driver(hierarchy, fn)
driver_proxy->>driver_proxy: make_driver_bundle / invoke_driver_with_sources
driver_proxy-->>load_driver: driver_bundle result
load_driver->>framework_graph: g.set_driver(result)
framework_graph->>framework_graph: validate mode + not-yet-set + non-empty, store driver_
Note over run,framework_graph: Providers and observers are wired after this point
run->>framework_graph: g.execute()
framework_graph->>framework_graph: assert driver_ configured
framework_graph->>layer_generator: driver_() — pull data cells via yielder
layer_generator-->>framework_graph: data_cell_index stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
21 fixed, 0 new since branch point (0de5b7b) ✅ 21 CodeQL alerts resolved since the previous PR commit
✅ 21 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@phlex/core/framework_graph.hpp`:
- Around line 79-83: The driver_proxy method currently forwards the result of
nodes_.sources_for(strings) directly without validating that all requested
source names were actually resolved, which can cause unresolved keys to be
silently dropped and lead to incorrect positional source binding or runtime
failures. Add validation logic within the driver_proxy method to check that the
collection returned by nodes_.sources_for(strings) contains exactly the number
of sources corresponding to the requested strings, and raise an error or
exception if any source names failed to resolve before constructing and
returning the experimental::driver_proxy object.
In `@phlex/core/node_catalog.cpp`:
- Around line 62-65: The sources_for() function silently skips missing source
keys in the loop instead of failing when a configured source name cannot be
found. Modify the conditional logic in the loop over keys so that when
sources.get(key) returns null or empty for any key, throw an exception with a
descriptive error message indicating which source name is missing, rather than
just skipping it with the if statement. This ensures configuration errors are
caught immediately instead of silently dropping unknown source names.
In `@phlex/driver.hpp`:
- Around line 114-128: The driver method that accepts a Generator parameter
needs to validate that no sources have been configured, similar to how the
function-driver overload validates this. Add a validation check at the beginning
of the driver method (before the null generator check) to ensure that sources_
is empty, and throw an std::invalid_argument exception with an appropriate
message if sources have been configured, since generator-style drivers do not
consume sources and would silently drop any configured source list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 36736741-8fb2-4555-a748-33dea01f4754
📒 Files selected for processing (34)
phlex/app/load_module.cppphlex/app/load_module.hppphlex/app/run.cppphlex/core/framework_graph.cppphlex/core/framework_graph.hppphlex/core/node_catalog.cppphlex/core/node_catalog.hppphlex/core/source.hppphlex/driver.hppphlex/model/fixed_hierarchy.cppphlex/model/fixed_hierarchy.hppplugins/generate_layers.cppplugins/layer_generator.cppplugins/layer_generator.hpptest/allowed_families.cpptest/cached_execution.cpptest/class_registration.cpptest/demo-giantdata/unfold_transform_fold.cpptest/filter.cpptest/fixed_hierarchy_test.cpptest/fold.cpptest/fold_duplicate_layer_name_test.cpptest/framework_graph.cpptest/function_registration.cpptest/hierarchical_nodes.cpptest/layer_generator.cpptest/memory-checks/many_events.cpptest/multiple_function_registration.cpptest/output_products.cpptest/product_selecting.cpptest/provider_test.cpptest/type_distinction.cpptest/unfold.cpptest/vector_of_abstract_types.cpp
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Framework-R-D/action-configure-cmake(auto-detected)Framework-R-D/action-workflow-setup(auto-detected)Framework-R-D/action-complete-pr-comment(auto-detected)Framework-R-D/action-handle-fix-commit(auto-detected)
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: build (gcc, none)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: coverage
- GitHub Check: clang-tidy-check
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,cc,cxx,h,hpp}: Use clang-format tool for all C++ code formatting (VS Code auto-formats on save); configuration defined in.clang-formatwith 100-character line limit and 2-space indentation
Follow clang-tidy recommendations defined in.clang-tidy
Files:
test/multiple_function_registration.cppplugins/generate_layers.cpptest/function_registration.cpptest/memory-checks/many_events.cpptest/hierarchical_nodes.cppphlex/model/fixed_hierarchy.hpptest/fold_duplicate_layer_name_test.cppphlex/core/node_catalog.hpptest/type_distinction.cpptest/product_selecting.cpptest/vector_of_abstract_types.cppphlex/core/source.hpptest/demo-giantdata/unfold_transform_fold.cpptest/output_products.cppphlex/core/node_catalog.cppphlex/app/load_module.cpptest/layer_generator.cpptest/unfold.cppphlex/app/load_module.hpptest/class_registration.cpptest/allowed_families.cpptest/fixed_hierarchy_test.cppplugins/layer_generator.cpptest/cached_execution.cpptest/fold.cppphlex/app/run.cpptest/framework_graph.cpptest/provider_test.cppphlex/core/framework_graph.hpptest/filter.cppphlex/model/fixed_hierarchy.cppphlex/core/framework_graph.cppphlex/driver.hppplugins/layer_generator.hpp
**/*.{hpp,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,cpp}: Use.hppfor header files,.cppfor implementation, and*_test.cppfor test files in C++
Enforce 100-character line limit and 2-space indentation in C++ code via.clang-format
UseQualifierAlignment: Right(east-const) style:int const xnotconst int xin C++
UsePointerAlignment: Leftin C++ (pointer*attached to type, not variable name)
All C++ identifiers must uselower_casenaming: namespaces, classes, structs, enums, functions, variables, parameters, members, and constants
Exception to C++ naming: template parameters useCamelCase
Exception to C++ naming: macros useUPPER_CASE
Private, protected, and constant members in C++ must have a trailing underscore (_), no trailing underscore on anything else
Useenum classpreferred over plainenumin C++
Usestd::shared_ptrfor shared ownership,std::unique_ptrfor exclusive ownership, raw pointers for non-owning references only in C++
Use functors with agent-noun pattern:ModelEvaluator evaluate_model(...)in C++
Apply.clang-tidychecks for bugprone, cert, clang-analyzer, concurrency, cppcoreguidelines, misc, modernize, performance, portability, and readability as defined in the.clang-tidyconfiguration file
Usephlex::namespace for core code,phlex::experimental::for experimental features in C++
Files:
test/multiple_function_registration.cppplugins/generate_layers.cpptest/function_registration.cpptest/memory-checks/many_events.cpptest/hierarchical_nodes.cppphlex/model/fixed_hierarchy.hpptest/fold_duplicate_layer_name_test.cppphlex/core/node_catalog.hpptest/type_distinction.cpptest/product_selecting.cpptest/vector_of_abstract_types.cppphlex/core/source.hpptest/demo-giantdata/unfold_transform_fold.cpptest/output_products.cppphlex/core/node_catalog.cppphlex/app/load_module.cpptest/layer_generator.cpptest/unfold.cppphlex/app/load_module.hpptest/class_registration.cpptest/allowed_families.cpptest/fixed_hierarchy_test.cppplugins/layer_generator.cpptest/cached_execution.cpptest/fold.cppphlex/app/run.cpptest/framework_graph.cpptest/provider_test.cppphlex/core/framework_graph.hpptest/filter.cppphlex/model/fixed_hierarchy.cppphlex/core/framework_graph.cppphlex/driver.hppplugins/layer_generator.hpp
**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Avoid boolean parameters in C++ interfaces; prefer enumerations instead
Files:
phlex/model/fixed_hierarchy.hppphlex/core/node_catalog.hppphlex/core/source.hppphlex/app/load_module.hppphlex/core/framework_graph.hppphlex/driver.hppplugins/layer_generator.hpp
🪛 Cppcheck (2.21.0)
plugins/generate_layers.cpp
[style] 26-26: The function 'PHLEX_REGISTER_DRIVER' is never used.
(unusedFunction)
phlex/app/load_module.cpp
[style] 116-116: The function 'load_driver' is never used.
(unusedFunction)
🔇 Additional comments (34)
phlex/model/fixed_hierarchy.hpp (1)
90-92: LGTM!phlex/model/fixed_hierarchy.cpp (1)
16-25: LGTM!Also applies to: 39-39, 51-64, 115-123
test/fixed_hierarchy_test.cpp (1)
74-80: LGTM!Also applies to: 82-93, 95-105
phlex/core/framework_graph.hpp (1)
29-29: LGTM!Also applies to: 42-63, 190-192, 201-201, 210-210
phlex/core/framework_graph.cpp (1)
18-34: LGTM!Also applies to: 53-79, 102-121
phlex/app/run.cpp (1)
20-20: LGTM!Also applies to: 41-43
test/layer_generator.cpp (1)
12-19: LGTM!Also applies to: 24-34, 38-50, 54-66, 70-77, 84-103
test/allowed_families.cpp (1)
37-43: LGTM!test/cached_execution.cpp (1)
55-61: LGTM!test/demo-giantdata/unfold_transform_fold.cpp (1)
48-54: LGTM!test/fold.cpp (1)
63-68: LGTM!Also applies to: 107-112
phlex/app/load_module.hpp (1)
25-25: LGTM!phlex/app/load_module.cpp (1)
116-127: LGTM!test/framework_graph.cpp (1)
11-11: LGTM!Also applies to: 16-17, 23-24, 30-31, 33-34, 45-45, 52-53, 55-56, 79-79, 90-91, 93-94, 114-114, 128-133, 135-156, 158-167
test/class_registration.cpp (1)
68-68: LGTM!test/function_registration.cpp (1)
64-64: LGTM!test/multiple_function_registration.cpp (1)
44-44: LGTM!test/type_distinction.cpp (1)
46-50: LGTM!test/vector_of_abstract_types.cpp (1)
42-46: LGTM!test/filter.cpp (1)
105-108: LGTM!Also applies to: 132-135, 155-158, 186-189, 213-216
test/fold_duplicate_layer_name_test.cpp (1)
60-66: LGTM!test/hierarchical_nodes.cpp (1)
81-86: LGTM!test/memory-checks/many_events.cpp (1)
17-21: LGTM!test/output_products.cpp (1)
39-43: LGTM!test/product_selecting.cpp (1)
31-34: LGTM!test/provider_test.cpp (1)
68-72: LGTM!Also applies to: 91-95, 111-111, 121-121, 140-140
test/unfold.cpp (1)
94-98: LGTM!Also applies to: 162-166
phlex/core/source.hpp (1)
46-46: LGTM!phlex/core/node_catalog.hpp (1)
32-33: LGTM!phlex/core/node_catalog.cpp (1)
4-4: LGTM!phlex/driver.hpp (1)
5-20: LGTM!Also applies to: 32-110, 132-163
plugins/layer_generator.hpp (1)
19-24: LGTM!Also applies to: 37-37, 50-50, 61-68
plugins/layer_generator.cpp (1)
14-17: LGTM!Also applies to: 139-146
plugins/generate_layers.cpp (1)
22-30: LGTM!Also applies to: 41-41
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #658 +/- ##
==========================================
+ Coverage 83.55% 83.83% +0.27%
==========================================
Files 170 170
Lines 6283 6366 +83
Branches 706 717 +11
==========================================
+ Hits 5250 5337 +87
+ Misses 810 804 -6
- Partials 223 225 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
knoepfel
force-pushed
the
update-hierarchy
branch
from
a08a772 to
175b150
Compare
knoepfel
force-pushed
the
update-hierarchy
branch
from
aeae51d to
a1eb67e
Compare
Contributor
Clang-Tidy Check ResultsFound 5504 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
Contributor
Clang-Tidy Check ResultsFound 5502 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
marcpaterno
reviewed
Jun 26, 2026
| class layer_generator : public std::enable_shared_from_this<layer_generator> { | ||
| public: | ||
| layer_generator(); | ||
| [[nodiscard]] static std::shared_ptr<layer_generator> make(); |
Member
There was a problem hiding this comment.
Consider a future modification of making make return a unique_ptr, which adds some flexibility and may allow other refactorings to simplify ownership. This should not block this PR.
marcpaterno
reviewed
Jun 26, 2026
| int max_parallelism = oneapi::tbb::info::default_concurrency()); | ||
| explicit framework_graph(driver_bundle bundle, | ||
| int max_parallelism = oneapi::tbb::info::default_concurrency()); | ||
| [[nodiscard]] static framework_graph with_default_driver( |
Member
There was a problem hiding this comment.
Consider renaming to without_driver, to help emphasize that the graph lacks a driver, rather than having one that is somehow a "deferred driver".
marcpaterno
reviewed
Jun 26, 2026
| .input_family(product_selector{.creator = "nonexistent_creator", .layer = "job"}); | ||
|
|
||
| CHECK_THROWS_WITH(g.execute(), | ||
| ContainsSubstring("No provider found for the following required products:") && |
Member
There was a problem hiding this comment.
Consider testing to ensure that the required product is listed in the expected format. This may be too much to deal with in release 0.3. If so, then consider creating an issue to say this should be done.
marcpaterno
requested changes
Jun 26, 2026
marcpaterno
left a comment
marcpaterno
left a comment
Member
There was a problem hiding this comment.
Please see the various comments. None are blockers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This PR updates deferred driver configuration so drivers can consume configured sources in a type-safe way and so driver-builder APIs are expressed with clearer concepts.
What Changed
Driver Function Concepts
is_driver_like_with_sources<F, FirstArg>to model driver callables that:data_cell_cursorordata_cell_yielder), andphlex::experimental::source.is_driver_like_with_cursoris_driver_like_with_yielderis_driver_builder_liketo require thatdriver_function()satisfies eitheris_driver_like_with_cursororis_driver_like_with_yielder.driver_proxySource-Aware Invocationdriver_proxynow invokes user driver functions with typed source parameters resolved from configured sources.detail::invoke_driver_with_sourcesandas_driver_source:Cannot configure driver with an empty driver_builder.Deferred Driver Wiring in
framework_graphexecute().driver_bundle, andhierarchy()anddriver_function().Layer Generator Integration
layer_generatoris used through shared ownership (make()factory) and exposesdriver_function()for graph driver registration.Tests
empty driver_builder).