Make layer specification optional - #817
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:
📝 WalkthroughWalkthroughThe PR adds optional layers to product selectors, treats unset layers as wildcards, validates mandatory layers, updates provider and unfold wiring, adds coverage for layerless selection, and adjusts GNU compiler-specific linkage. ChangesLayerless product selection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant IndexRouter
participant MakeComputationalEdges
participant ProductSelector
IndexRouter->>MakeComputationalEdges: Register provider inputs
MakeComputationalEdges->>ProductSelector: Match layerless selector
ProductSelector-->>MakeComputationalEdges: Return matching provider
MakeComputationalEdges-->>IndexRouter: Apply provider layer or wildcard identifier
Possibly related PRs
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 |
![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.cpp`:
- Around line 69-72: Update framework_graph::finalize() to compute unfold
validation/data before the early return triggered by empty head_ports. Ensure
the resulting unfold information is passed to index_router_.finalize(), and
preserve validation of each unfold input’s layer, including the check in the
unfold handling around input.layer and n->name().
In `@phlex/core/multilayer_join_node.hpp`:
- Line 79: In the multilayer join setup around set_ports, validate that
layers_.size() is at least NInputs before indexing repeaters_[Is]. Restore the
equivalent assertion or throw a clear validation error, while preserving the
existing behavior for valid layer counts.
In `@phlex/core/products_consumer.hpp`:
- Around line 22-25: Replace the boolean layer-policy parameter in
products_consumer with a lower_case enum class and named enumerators; update
products_consumer.cpp to compare against the appropriate enum value, and update
declared_fold.cpp to pass the mandatory-layers enumerator. Apply these changes
at phlex/core/products_consumer.hpp lines 22-25,
phlex/core/products_consumer.cpp lines 25-49, and phlex/core/declared_fold.cpp
lines 8-11.
🪄 Autofix
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: bdd3e12f-a8d6-4970-8b88-c30859ed4aab
📒 Files selected for processing (11)
phlex/core/CMakeLists.txtphlex/core/declared_fold.cppphlex/core/framework_graph.cppphlex/core/index_router.cppphlex/core/make_computational_edges.cppphlex/core/multilayer_join_node.hppphlex/core/product_selector.cppphlex/core/product_selector.hppphlex/core/products_consumer.cppphlex/core/products_consumer.hpptest/product_selecting.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: clang-tidy-check
- GitHub Check: build (gcc, none)
- GitHub Check: coverage
🧰 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:
phlex/core/declared_fold.cppphlex/core/products_consumer.hppphlex/core/make_computational_edges.cppphlex/core/framework_graph.cppphlex/core/index_router.cppphlex/core/multilayer_join_node.hppphlex/core/products_consumer.cppphlex/core/product_selector.cpptest/product_selecting.cppphlex/core/product_selector.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:
phlex/core/declared_fold.cppphlex/core/products_consumer.hppphlex/core/make_computational_edges.cppphlex/core/framework_graph.cppphlex/core/index_router.cppphlex/core/multilayer_join_node.hppphlex/core/products_consumer.cppphlex/core/product_selector.cpptest/product_selecting.cppphlex/core/product_selector.hpp
**/*.hpp
📄 CodeRabbit inference engine (AGENTS.md)
Avoid boolean parameters in C++ interfaces; prefer enumerations instead
Files:
phlex/core/products_consumer.hppphlex/core/multilayer_join_node.hppphlex/core/product_selector.hpp
🪛 Cppcheck (2.21.0)
test/product_selecting.cpp
[style] 60-60: The function 'initial_value' is never used.
(unusedFunction)
🔇 Additional comments (10)
test/product_selecting.cpp (1)
4-7: LGTM!Also applies to: 31-66, 76-76, 92-106, 115-125, 127-143, 145-159, 161-174, 176-191
phlex/core/product_selector.hpp (2)
12-14: LGTM!Also applies to: 47-77, 80-98
79-79: 🩺 Stability & AvailabilityDo not change
layer_name::operator*in this patch.layer_nameis intentionally optional, and its unchecked access matchesstd::optional::operator*. In-tree callers guard it before dereferencing. This patch only adds defaulted comparisons.> Likely an incorrect or invalid review comment.phlex/core/product_selector.cpp (1)
12-12: LGTM!Also applies to: 53-53, 80-80, 102-103
phlex/core/CMakeLists.txt (1)
42-48: LGTM!Also applies to: 110-116
phlex/core/products_consumer.cpp (1)
3-18: LGTM!phlex/core/make_computational_edges.cpp (2)
55-60: LGTM!Also applies to: 123-128
86-86: 🗄️ Data Integrity & IntegrationNo change needed
product_selector::matchtreats an unset layer on the left as a wildcard and rejects a different concrete layer. Existing tests cover layerless requests and shared implicit providers.phlex/core/index_router.cpp (2)
110-110: 🎯 Functional CorrectnessNo change needed for
_idlookup.phlex/core/product_selector.hppis transitively included beforewire_provider_index_sets, and its namespace-scope using-directive makes"_id"visible at Line 201.> Likely an incorrect or invalid review comment.
201-201: 🗄️ Data Integrity & IntegrationNo wildcard precedence change is required. The selector layer is populated from the matched provider before registration, and
"*"_iddoes not match concrete layer names.> Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #817 +/- ##
==========================================
+ Coverage 83.89% 84.01% +0.11%
==========================================
Files 171 172 +1
Lines 7234 7475 +241
Branches 850 887 +37
==========================================
+ Hits 6069 6280 +211
- Misses 881 901 +20
- Partials 284 294 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Exercise omitted-layer resolution across computational nodes and explicit and implicit providers. Verify that unfolds reject selectors without a layer using the intended diagnostic. AI-Authored-By: OpenAI Codex AI-Model: gpt-5.6-sol AI-Reasoning-Effort: medium
Select the libstdc++ stacktrace support library once from the compiler and version, then expose it through both core targets. This keeps the generated link interfaces complete for GCC and Clang without extra configuration probes. AI-Authored-By: OpenAI Codex AI-Model: gpt-5.6-sol AI-Reasoning-Effort: medium
|
@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: 3fc4abb7a2
ℹ️ 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".
2 participants
Layer specifications are now optional in
product_selectors. As discussed with Kyle and Marc, this is currently limited to single input functions. Folds are always considered multi-input (they have an implicit accumulator input).In addition, this is not currently enabled for unfolds because their input layers are used in the flush machinery.
AI summary below
Core behavior
product_selectorlayers for single-input functions.Implementation
product_selector.products_consumerto enforce layers for multi-input consumers.Tests