Express fold nodes using accumulator nodes#540
Open
knoepfel wants to merge 7 commits into
Open
Conversation
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #540 +/- ##
==========================================
+ Coverage 83.27% 84.09% +0.81%
==========================================
Files 162 167 +5
Lines 5912 6625 +713
Branches 670 789 +119
==========================================
+ Hits 4923 5571 +648
- Misses 796 816 +20
- Partials 193 238 +45
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
knoepfel
force-pushed
the
fix-fold-caching
branch
2 times, most recently
from
7a865dc to
3d19c63
Compare
knoepfel
force-pushed
the
fix-fold-caching
branch
from
3d19c63 to
530eb15
Compare
Contributor
|
31 fixed, 0 new since branch point (0f6474d) ✅ 31 CodeQL alerts resolved since the previous PR commit
✅ 31 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
knoepfel
force-pushed
the
fix-fold-caching
branch
from
530eb15 to
c3caf2f
Compare
knoepfel
force-pushed
the
fix-fold-caching
branch
from
c3caf2f to
197de5e
Compare
knoepfel
force-pushed
the
fix-fold-caching
branch
from
197de5e to
10aac8e
Compare
knoepfel
force-pushed
the
fix-fold-caching
branch
3 times, most recently
from
322927f to
decf3ae
Compare
knoepfel
force-pushed
the
fix-fold-caching
branch
from
decf3ae to
e7eb445
Compare
knoepfel
force-pushed
the
fix-fold-caching
branch
2 times, most recently
from
e564fb3 to
5471faa
Compare
knoepfel
force-pushed
the
fix-fold-caching
branch
2 times, most recently
from
34e1b5e to
e106e73
Compare
|
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:
📝 WalkthroughWalkthroughReplaces the ChangesFold pipeline refactor (linked to
Sequence Diagram(s)sequenceDiagram
participant FG as framework_graph
participant IR as index_router
participant FJN as fold_join_node
participant AN as accumulator_node
participant FoldMFN as fold multifunction_node
rect rgba(100, 149, 237, 0.5)
note over FG,IR: Graph finalization
FG->>IR: finalize(layer_paths, unfold_data, provider_ports, fold_partition_ports, join_ports)
IR->>IR: establish_layer_hierarchy, precompute is_lowest_layer_hashes_
IR->>IR: wire_provider_index_sets, wire_fold_partition_index_sets
IR->>IR: build_multilayer_join_slots with flush_specs
end
rect rgba(60, 179, 113, 0.5)
note over IR,FoldMFN: Runtime fold execution
IR->>AN: index_message to partition_port
AN->>AN: initialize cached_accumulator, emit pending ids
IR->>FJN: index_message to data repeater port
FJN->>FoldMFN: joined tuple of accumulator_message and input product
FoldMFN->>AN: notify_result_repeater_port, increment call counter
IR->>AN: indexed_end_token to flush_port
AN->>AN: decrement counter, check flush received
AN-->>FJN: release_as_message when counter is zero and flush received
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/concepts.hpp`:
- Around line 20-21: The `sendable` concept's constraint `sendable_type<T>{t}`
is ill-formed for `std::atomic<T>` types because direct brace-initialization
from an atomic reference is not allowed (e.g., `int{atomic_const_ref}` fails).
Fix this by reverting to the previous constraint approach that used `{ send(t) }
-> std::move_constructible`, which properly handles type conversion through the
`send()` function, or alternatively verify that `sendable_type<T>` exists
without attempting direct initialization.
In `@phlex/core/declared_fold.hpp`:
- Line 149: Remove the unused member variable `initializer_` from the class
definition in declared_fold.hpp. This member variable is default-constructed and
never referenced in the class implementation, making it leftover code. The
`initializer` constructor parameter is already being passed directly to
`make_initializer` and `join_`, so the member variable is not needed. Simply
delete the `InitTuple initializer_;` declaration line.
- Line 157: The member variable `product_count_` in the class is declared with
an accessor but never incremented, unlike the similar implementations in
declared_unfold.hpp and declared_transform.hpp. Either remove the
`product_count_` member variable and its corresponding accessor method if this
metric is not applicable to the fold pipeline design, or implement the logic to
increment `product_count_` at the appropriate point in the fold operation
processing to ensure it actually tracks the product count as intended,
maintaining consistency with the other declared pipeline classes.
In `@phlex/core/detail/accumulator_node.hpp`:
- Around line 295-304: The cleanup_cache_entry method dereferences
entry->accumulator_msg without checking if it is null. When flush_received is
true and counter is zero, but the partition message has not yet arrived,
accumulator_msg remains null, causing a null pointer dereference. Add a null
pointer check for entry->accumulator_msg before calling release_as_message on
it, and only proceed with the try_put and cached_results_.erase operations if
accumulator_msg is not null.
In `@phlex/core/fold_join_node.hpp`:
- Line 67: Fix the typo in the comment on line 67 of the fold_join_node.hpp
file. Change "qacross" to "across" in the comment that describes message
matching behavior. The corrected comment should read "Messages with the same ID
across" instead of "Messages with the same ID qacross".
In `@phlex/core/fold/send.hpp`:
- Around line 27-38: The two specializations of sendable_type_impl can become
ambiguous if a type is both std::move_constructible and has a valid send()
overload. To make these specializations mutually exclusive, add a requires
clause to the first sendable_type_impl specialization (the one that matches
std::move_constructible T) that explicitly excludes types satisfying the send()
requirement. This requires clause should check that the type does not satisfy
the condition in the second specialization, ensuring only one specialization can
match for any given type.
🪄 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: 26a2d1c9-a41c-4558-90d8-009f33711ab4
📒 Files selected for processing (25)
.amazonq/rules/memory-bank/structure.mdphlex/core/CMakeLists.txtphlex/core/concepts.hppphlex/core/declared_fold.cppphlex/core/declared_fold.hppphlex/core/declared_observer.hppphlex/core/declared_unfold.cppphlex/core/declared_unfold.hppphlex/core/detail/accumulator_node.hppphlex/core/fold/send.hppphlex/core/fold_join_node.hppphlex/core/framework_graph.cppphlex/core/framework_graph.hppphlex/core/fwd.hppphlex/core/index_router.cppphlex/core/index_router.hppphlex/core/message.hppphlex/core/multilayer_join_node.hppphlex/core/store_counters.cppphlex/core/store_counters.hppphlex/model/flush_gate.cppphlex/model/flush_gate.hpptest/CMakeLists.txttest/accumulator_test.cpptest/flush_gate_test.cpp
💤 Files with no reviewable changes (6)
- phlex/core/declared_observer.hpp
- phlex/core/store_counters.hpp
- phlex/core/CMakeLists.txt
- phlex/core/store_counters.cpp
- phlex/core/framework_graph.hpp
- phlex/core/fwd.hpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (gcc, none)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: clang-tidy-check
- GitHub Check: coverage
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
phlex/core/fold/send.hppphlex/core/declared_unfold.cppphlex/core/declared_fold.cppphlex/core/concepts.hppphlex/core/multilayer_join_node.hppphlex/model/flush_gate.hppphlex/core/declared_unfold.hpptest/flush_gate_test.cppphlex/core/fold_join_node.hppphlex/core/detail/accumulator_node.hpptest/accumulator_test.cppphlex/model/flush_gate.cppphlex/core/framework_graph.cppphlex/core/message.hppphlex/core/index_router.hppphlex/core/declared_fold.hppphlex/core/index_router.cpp
**/*.{hpp,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,cpp}: Use.hppfor C++ header files and.cppfor implementation files; test files should be named*_test.cpp
All C++ identifiers (namespaces, classes, structs, enums, functions, variables, parameters, members, constants, type aliases) must uselower_casenaming
Template parameters in C++ must useCamelCasenaming
C++ macros must useUPPER_CASEnaming
Private, protected, and constant C++ members must have a trailing underscore (_)
Useeast-conststyle in C++:int const xnotconst int x
Preferenum classover plainenumin C++
Avoid boolean parameters in C++ function interfaces; prefer enumerations instead
Usestd::shared_ptrfor shared ownership,std::unique_ptrfor exclusive ownership, and raw pointers only for non-owning references in C++
C++ clang-format enforces 100-character line limit and 2-space indentation withQualifierAlignment: RightandPointerAlignment: Left
Use namespacesphlex::for core code andphlex::experimental::for experimental features in C++
Use functors with agent noun naming (e.g.,ModelEvaluator evaluate_model(...)) in C++
Files:
phlex/core/fold/send.hppphlex/core/declared_unfold.cppphlex/core/declared_fold.cppphlex/core/concepts.hppphlex/core/multilayer_join_node.hppphlex/model/flush_gate.hppphlex/core/declared_unfold.hpptest/flush_gate_test.cppphlex/core/fold_join_node.hppphlex/core/detail/accumulator_node.hpptest/accumulator_test.cppphlex/model/flush_gate.cppphlex/core/framework_graph.cppphlex/core/message.hppphlex/core/index_router.hppphlex/core/declared_fold.hppphlex/core/index_router.cpp
🪛 Cppcheck (2.20.0)
test/accumulator_test.cpp
[style] 24-24: The function 'convert' is never used.
(unusedFunction)
🔇 Additional comments (29)
phlex/core/detail/accumulator_node.hpp (1)
1-186: LGTM!Also applies to: 206-237, 239-293
phlex/core/fold_join_node.hpp (1)
1-66: LGTM!Also applies to: 68-171, 173-233
phlex/core/declared_fold.hpp (1)
38-54: LGTM!Also applies to: 59-70, 74-148, 150-156, 158-158
phlex/core/declared_fold.cpp (1)
1-15: LGTM!phlex/core/message.hpp (1)
52-77: LGTM!phlex/core/multilayer_join_node.hpp (1)
114-117: LGTM!phlex/core/index_router.hpp (1)
27-65: LGTM!Also applies to: 85-98, 103-125, 148-184, 190-226
phlex/core/index_router.cpp (8)
45-106: LGTM!
108-126: LGTM!
128-143: LGTM!
199-258: LGTM!
362-450: LGTM!
452-513: LGTM!
577-611: LGTM!
175-175: This is not a concern—the project explicitly targets C++23 as its baseline standard (per AGENTS.md and CLANG_TIDY_CONFIGURATION.md), andstd::ranges::containsis already used elsewhere in the codebase without issue (e.g., phlex/model/fixed_hierarchy.cpp:24). No action needed.phlex/core/framework_graph.cpp (4)
17-41: LGTM!
43-75: LGTM!
218-223: LGTM!
122-128: LGTM!phlex/core/declared_unfold.hpp (1)
45-55: LGTM!test/accumulator_test.cpp (3)
20-80: LGTM!
81-117: LGTM!
120-268: LGTM!phlex/core/declared_unfold.cpp (1)
23-23: LGTM!test/CMakeLists.txt (1)
14-14: LGTM!test/flush_gate_test.cpp (1)
29-29: LGTM!Also applies to: 225-231
.amazonq/rules/memory-bank/structure.md (1)
19-19: LGTM!phlex/model/flush_gate.hpp (1)
43-43: LGTM!Also applies to: 61-61, 70-70, 88-88
phlex/model/flush_gate.cpp (1)
11-11: LGTM!Also applies to: 22-22, 28-28, 38-42, 86-86
knoepfel
force-pushed
the
fix-fold-caching
branch
2 times, most recently
from
e47a8b2 to
979191e
Compare
* Includes adjusting infrastructure for "sendable" types
Also remove unnecessary infrastructure
knoepfel
force-pushed
the
fix-fold-caching
branch
from
dd941a9 to
c7374d2
Compare
Contributor
Clang-Tidy Check ResultsFound 6282 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.
This PR replaces the caching mechanism for fold results with a similar mechanism as used in #118.
Resolves #359
Code Changes
Core Fold Mechanism Refactoring
accumulator_node<Result>for per-partition result caching with concurrent cache managementfold_join_nodecomposite node that wires accumulator nodes with repeater nodes via TBB join logicdeclared_foldandfold_nodeto use partition-layer routing model instead of flush portssendable_typeconcept with new trait-based implementation supporting both move-constructible and sendable-wrapped typesIndex Router Refactoring
multilayer_slotto route index messages only (removed end-token handling)establish_layers()+register_unfold_count_per_input_layer()) with unifiedfinalize()callestablish_layer_hierarchy()to compute layer relationships and unfold metadata with fixed-point calculationmultilayer_slots_for()return type to emit end-token entries per slot rather than separate slot referencescounting_layer_hashes_under()anddeepest_layer_name()for hierarchy traversalInfrastructure & Model Updates
flush_gateto storecommitted_counts_as value-baseddata_cell_countsinstead ofstd::shared_ptrflush_gatemethod signatures:committed_counts()androll_up_child()now use reference semanticsnamed_index_portwithcounting_layerfield to distinguish routing layer from counting layerflush_messagestruct entirelyflusher_ttype alias and associated TBB flow-graph dependency fromphlex/core/fwd.hppAPI Removals & Cleanup
store_counters.hppandstore_counters.cpp(cache management logic now inaccumulator_node)index_router:establish_layers(),register_unfold_count_per_input_layer(),flusher()accessorfold_nodeinheritance fromcount_storesclassdeclared_observer.hppto removestore_counters.hppincludeVariable/Member Cleanup
generator::child_counts_tochild_count_indeclared_unfoldfor naming consistencymake_initializer()to only capture args when index pack is non-emptyBuild System
store_counters.cppfromphlex/core/CMakeLists.txtstore_counters.hppfrom public header installationaccumulatorCTest target using Catch2 (links againstphlex::core_internal)Tests
accumulator_test.cppsuite covering:flush_gate_test.cppto use value-baseddata_cell_counts(removed heap allocation pattern)Documentation
.amazonq/rules/memory-bank/structure.mdto referenceproduct_selector.cpp/hppinstead ofproduct_query.cpp/hppunderphlex/coredirectory organization