Skip to content

fix: fold operations must use at least one data product - #1005

Merged
marcpaterno merged 2 commits into
Framework-R-D:mainfrom
knoepfel:fix/constrain-fold-signatures
Oct 22, 2026
Merged

marcpaterno merged 2 commits into
Framework-R-D:mainfrom
knoepfel:fix/constrain-fold-signatures

Conversation

@knoepfel

@knoepfel knoepfel commented Oct 21, 2026 •

Copy link
Copy Markdown
Member
  • Code

    • Require fold operations to include at least one data product.
    • Reject accumulator-only and resource-only fold signatures at compile time.
    • Update fold registration diagnostics to report data-product, accumulator, and resource counts.
    • Track data-product counts separately from total fold arguments in fold-join templates.
    • Remove the empty-layer fallback because fold inputs are now always non-empty.
    • Use GSL contract checks and reserve repeater capacity based on the data-product count.
  • Tests and build

    • Add compile-failure tests for accumulator-only and resource-only folds.
    • Add CMake support for compile-failure tests.
    • Verify that each invalid fold fails and emits the expected diagnostic.

@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: e932497a-55c4-4d4b-82c2-652cb510ea86

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: cd4aa32a-fbc5-4f66-adb4-7767eae88a1c
📥 Commits

Reviewing files that changed from the base of the PR and between a268841 and 64e1484.

📒 Files selected for processing: 7
  • phlex/core/fold_join_node.hpp
  • phlex/core/registration_api.hpp
  • test/CMakeLists.txt
  • test/compile-fail/CMakeLists.txt
  • test/compile-fail/check_compile_failure.cmake
  • test/compile-fail/fold_accumulator_only_test.cpp
  • test/compile-fail/fold_resource_only_test.cpp

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout: 4
  • GitHub Check: Analyze cpp with CodeQL
  • GitHub Check: build (gcc, none)
  • GitHub Check: clang-tidy-check
  • GitHub Check: coverage
🧰 Additional context used
📚 Code guidelines: 1
.github/copilot-instructions.md — auto-discovered
📓 Path-based instructions: 2
Source excerpt: Use cmake-format tool (VS Code auto-formats on save)

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Files:

  • test/compile-fail/check_compile_failure.cmake
Source excerpt: Use clang-format tool for all C++ code (VS Code auto-formats on save)

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Files:

  • test/compile-fail/fold_accumulator_only_test.cpp
  • phlex/core/registration_api.hpp
  • test/compile-fail/fold_resource_only_test.cpp
  • phlex/core/fold_join_node.hpp
🔇 Additional comments: 12
phlex/core/registration_api.hpp: 2

266-267: LGTM!

297-306: LGTM!

phlex/core/fold_join_node.hpp: 5

8-8: LGTM!

39-62: LGTM!

91-101: LGTM!

115-115: LGTM!

135-150: LGTM!

test/CMakeLists.txt: 1

213-213: LGTM!

test/compile-fail/CMakeLists.txt: 1

1-23: LGTM!

test/compile-fail/check_compile_failure.cmake: 1

1-21: LGTM!

test/compile-fail/fold_accumulator_only_test.cpp: 1

1-7: LGTM!

test/compile-fail/fold_resource_only_test.cpp: 1

1-22: LGTM!

📝 Walkthrough

Fold registrations and fold-join nodes now require at least one data product. The pull request adds compile-failure tests for accumulator-only and resource-only folds and checks their expected diagnostics.

Changes: Fold product-input constraints

Layer / File(s) Summary
Fold registration constraints
phlex/core/registration_api.hpp
Fold registration now requires at least one data product and provides fold-specific input-count diagnostics that identify the accumulator, products, and resources.
Fold-join product-count and partition setup
phlex/core/fold_join_node.hpp
The fold-join template rejects zero products, validates the product-layer count with GSL contract checking, and uses layers_[0] for partition index-port counting.
Compile-failure checks for productless folds
test/CMakeLists.txt, test/compile-fail/*
The test build now includes compile-failure checks for accumulator-only and resource-only folds. The checks require compilation to fail and verify the expected diagnostic.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix

Possibly related issues

  • Issue 998 proposes separate leading accumulator arguments while retaining data-product and resource arguments. This pull request adds a data-product requirement but does not implement separate accumulator arguments.

Suggested reviewers: beojan

Merge Risk: ⚪ Minimal · up to 64e14

The new product-input constraint and its tests are consistent across registration and fold-join construction. No merge-blocking issue was identified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the primary change: fold operations now require at least one data product.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (3 skipped: 3 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             main    #1005      +/-   ##
==========================================
+ Coverage   89.16%   89.28%   +0.12%     
==========================================
  Files         176      176              
  Lines        6061     6064       +3     
  Branches     1012     1011       -1     
==========================================
+ Hits         5404     5414      +10     
+ Misses        410      407       -3     
+ Partials      247      243       -4     
Flag Coverage Δ
amd64 89.28% <100.00%> (+0.12%) ⬆️
arm64 89.28% <100.00%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
phlex/core/fold_join_node.hpp 93.61% <100.00%> (+1.78%) ⬆️
phlex/core/registration_api.hpp 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d1df5d...c1d7d6a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@knoepfel
knoepfel force-pushed the fix/constrain-fold-signatures branch from 64e1484 to cfa0d06 Compare October 21, 2026 18:43
@knoepfel
knoepfel requested a review from marcpaterno October 21, 2026 20:39
Reject accumulator-only and resource-only folds, and distinguish the
product count from the total argument count in fold-join templates.

Use the nonempty-input invariant to remove the empty-layer fallback.
Invocation-domain selection follows in the shared join-registration change.
@knoepfel
knoepfel force-pushed the fix/constrain-fold-signatures branch from cfa0d06 to c1d7d6a Compare October 22, 2026 00:08
@marcpaterno
marcpaterno merged commit 24acef6 into Framework-R-D:main Oct 22, 2026
42 checks passed
@knoepfel
knoepfel deleted the fix/constrain-fold-signatures branch October 22, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants