Skip to content

Add type checking to graph construction#119

Merged
knoepfel merged 30 commits intoFramework-R-D:mainfrom
beojan:graph-type-checking
Dec 2, 2025
Merged

Add type checking to graph construction#119
knoepfel merged 30 commits intoFramework-R-D:mainfrom
beojan:graph-type-checking

Conversation

@beojan
Copy link
Contributor

@beojan beojan commented Nov 17, 2025

Add type checking to products (at least when registering algorithms in C++).

The qualified_name for output products, and the specified_label for input products now have type information attached (in the form of a custom type_id currently calculated mostly at compile time) that is compared in find_products.

I had to use a custom type_id so we could do things like treating handle<int>, int const&, and int const* as the same type. This should also simplify extending this type checking to Python algorithms.

I think I've done the best I can with code coverage. I don't know why it thinks the formatting code isn't covered, it definitely should be in my type_id.cpp test.

@codecov
Copy link

codecov bot commented Nov 17, 2025

Codecov Report

❌ Patch coverage is 85.99034% with 29 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
phlex/core/edge_creation_policy.cpp 81.81% 6 Missing ⚠️
phlex/model/type_id.hpp 92.77% 2 Missing and 4 partials ⚠️
phlex/core/declared_transform.cpp 0.00% 2 Missing and 1 partial ⚠️
phlex/core/declared_unfold.cpp 0.00% 3 Missing ⚠️
phlex/core/product_query.cpp 80.00% 3 Missing ⚠️
phlex/core/product_query.hpp 75.00% 2 Missing and 1 partial ⚠️
phlex/model/product_specification.cpp 88.46% 2 Missing and 1 partial ⚠️
phlex/model/product_specification.hpp 75.00% 2 Missing ⚠️
@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
+ Coverage   80.31%   81.29%   +0.97%     
==========================================
  Files         115      116       +1     
  Lines        1910     2042     +132     
  Branches      302      326      +24     
==========================================
+ Hits         1534     1660     +126     
  Misses        247      247              
- Partials      129      135       +6     
Flag Coverage Δ
unittests 81.29% <85.99%> (+0.97%) ⬆️

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

Files with missing lines Coverage Δ
phlex/core/declared_fold.cpp 100.00% <ø> (ø)
phlex/core/declared_fold.hpp 92.98% <100.00%> (ø)
phlex/core/declared_observer.cpp 33.33% <ø> (ø)
phlex/core/declared_observer.hpp 100.00% <ø> (ø)
phlex/core/declared_predicate.cpp 50.00% <ø> (ø)
phlex/core/declared_predicate.hpp 89.65% <ø> (ø)
phlex/core/declared_transform.hpp 91.48% <100.00%> (+0.18%) ⬆️
phlex/core/declared_unfold.hpp 94.54% <100.00%> (+0.20%) ⬆️
phlex/core/detail/filter_impl.cpp 92.50% <ø> (ø)
phlex/core/detail/filter_impl.hpp 100.00% <ø> (ø)
... and 21 more

Continue to review full report in Codecov by Sentry.

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

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

@beojan beojan requested a review from knoepfel November 20, 2025 19:52
@knoepfel knoepfel linked an issue Nov 20, 2025 that may be closed by this pull request
Copy link
Member

@knoepfel knoepfel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beojan, bravo. I think you have hit all of the spots in the implementation that would be affected by imposing type constraints when establishing edges. I have several comments below for your consideration and for further discussion. A great start—thanks.

@beojan beojan force-pushed the graph-type-checking branch from a711c33 to 10e3f6e Compare November 21, 2025 16:04
@beojan
Copy link
Contributor Author

beojan commented Nov 25, 2025

I ran the coverage script locally using the LLVM backend and that shows line coverage for type_id.hpp at 96.9%

image

@beojan beojan force-pushed the graph-type-checking branch from 4a0904e to 76905a3 Compare November 25, 2025 15:58
@beojan
Copy link
Contributor Author

beojan commented Nov 25, 2025

OK, should be ready now. I have some ideas for further cleaning up find_producer but that's for another PR.

@beojan beojan marked this pull request as ready for review November 25, 2025 16:36
Copy link
Member

@knoepfel knoepfel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beojan, apologies for the delay in reviewing this. I think the highest priority item is to implement a unit test verifying that nodes with mismatched types are not connected (see below). Happy to discuss any of my comments.

@beojan beojan force-pushed the graph-type-checking branch from e4f9b70 to 07a8820 Compare December 2, 2025 00:11
beojan and others added 9 commits December 2, 2025 08:22
This fits better with the new class names, and should make it easier to
extend the set of properties on which we match later
Not supported in CI
clang-tidy still complains, but I think this looks a bit better
Co-authored-by: Kyle Knoepfel <knoepfel@fnal.gov>
@beojan beojan force-pushed the graph-type-checking branch from 07a8820 to ebc9771 Compare December 2, 2025 16:22
@knoepfel knoepfel merged commit d5938fd into Framework-R-D:main Dec 2, 2025
41 checks passed
@beojan beojan deleted the graph-type-checking branch December 2, 2025 19:11
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.

Use programming "type" when connecting edges

2 participants