Allow drivers to access configured sources#658
Open
knoepfel wants to merge 7 commits into
Open
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:
📝 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
|
31 fixed, 0 new since branch point (cb81c8b) ✅ 31 CodeQL alerts resolved since the previous PR commit
✅ 31 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}
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
Comment on lines
+114
to
+128
| driver_bundle driver(std::shared_ptr<Generator> generator) const | ||
| { | ||
| if (!generator) { | ||
| throw std::invalid_argument("Cannot configure driver with an empty generator."); | ||
| } | ||
|
|
||
| auto hierarchy = generator->hierarchy(); | ||
| auto generator_driver = generator->driver_function(); | ||
|
|
||
| return {[generator = std::move(generator), | ||
| h = hierarchy, | ||
| generator_driver = std::move(generator_driver)](framework_driver& d) mutable { | ||
| std::invoke(generator_driver, h.yielder(d)); | ||
| }, | ||
| std::move(hierarchy)}; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject configured sources for generator drivers until they’re supported.
This overload never consumes sources_; if uses_sources is supplied for a generator-style driver, the configured source list is silently dropped. Mirror the function-driver validation by rejecting non-empty sources_ here.
🐛 Proposed fix
if (!generator) {
throw std::invalid_argument("Cannot configure driver with an empty generator.");
}
+ if (!sources_.empty()) {
+ throw std::invalid_argument(
+ "Generator drivers do not accept configured source parameters.");
+ }
auto hierarchy = generator->hierarchy();🤖 Prompt for 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.
In `@phlex/driver.hpp` around lines 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.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #658 +/- ##
==========================================
+ Coverage 83.27% 83.72% +0.45%
==========================================
Files 162 170 +8
Lines 5912 6679 +767
Branches 670 807 +137
==========================================
+ Hits 4923 5592 +669
- Misses 796 816 +20
- Partials 193 271 +78
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 17 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
Contributor
Clang-Tidy Check ResultsFound 5580 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
Contributor
Clang-Tidy Check ResultsFound 5580 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
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
2 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
Previously, drivers were constructed before the
framework_graphwas initialized, which meant they had no way to access the sources (input data providers) that had been configured for the graph. This PR wires drivers into the graph after sources are registered, so that a driver can query which sources it is supposed to drive.What Changed
Driver Configuration Model
framework_graphnow uses two static factory methods instead of direct constructors:with_default_driver(int max_parallelism)with_deferred_driver(int max_parallelism)add_driver()beforeexecute()The internal driver storage is now
std::optional<framework_driver>(previously a bareframework_drivermember), and adriver_mode_enum tracks which mode the graph is in.Drivers Can Declare Source Parameters
Drivers can now accept parameters derived from
sourcealongside their primary cursor/yielder argument. The framework resolves these parameters automatically from the configured source list:The new
detail::invoke_driver_with_sourceshelper unpacks asource_vectorinto the typed parameters viadynamic_castand compile-time index sequencing. Theis_driver_like_with_cursorandis_driver_like_with_yielderconcepts are now defined in terms of the newis_driver_like_with_sourcesconcept, which validates that additional parameters aresource-derived.load_driverRefactoringload_drivernow accepts aframework_graph&and registers the driver directly instead of returning adriver_bundle. It reads an optionaluses_sourceslist from the configuration and passes the resolved sources to the driver viag.driver_proxy(required_sources).New
framework_graphHelpersadd_driver(driver_bundle)— deferred driver registration (validates mode, rejects duplicates)add_driver(std::shared_ptr<Generator>)— templated overload constrained byis_driver_generator_likedriver_proxy(std::vector<std::string> strings = {})— convenience helper that resolves source names throughnode_catalog::sources_for()execute()now throws if no driver has been configuredSource Infrastructure
source_vectortype alias:std::vector<source const*>node_catalog::sources_for(std::vector<std::string> const& keys)builds asource_vectorfrom named keysLayer Generator Refactoring
layer_generatorcan no longer be stack-constructed. Use the new factory:The new
driver_function()method returns astd::function<void(data_cell_yielder)>suitable for use as a driver callback. Thedriver_for_testhelper has been removed.Fixed Hierarchy Updates
fixed_hierarchy::update(std::vector<layer_path>)merges additional layer paths while ignoring duplicates and recomputes the hash tableTests
All ~30 affected test files have been updated to the new driver initialization pattern. New test coverage includes:
fixed_hierarchyconstructor deduplication andupdate()behaviorexecute()is called without a configured driverset_driver)set_drivercalls