-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable Fluxion resource graph elasticity #989
Conversation
3bbb7d8
to
8426d27
Compare
08d5577
to
fe2efb6
Compare
acd46c1
to
3349b69
Compare
3349b69
to
8e77db9
Compare
@JaeseungYeom and @tpatki the PR is ready for review. |
665ae76
to
52ac67f
Compare
Adding the conditional pragma with the suppression directive works for the Clang and GCC 12 tests but breaks the earlier GCC tests. That's because I added additional checks for |
That sounds plenty good enough. Sorry that was a bit of a siege, we’ll get everything straightened out so we don’t need to deal with this kind of thing in external deps again.
---
Sent from Workspace ONE Boxer<https://whatisworkspaceone.com/boxer>
On August 20, 2023 at 3:58:17 PM PDT, Daniel Milroy ***@***.***> wrote:
Adding the conditional pragma with the suppression directive works for the Clang and GCC 12 tests but breaks the earlier GCC tests. That's because #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" and -Wmissing-template-keyword are unrecognized directives for earlier versions of GCC (or conflict with -Wno-error=maybe-uninitialized from generate-matrix.py), but seem to be required for later versions (i.e., -Wno-error=maybe-uninitialized" doesn't work for GCC 12).
I added additional checks for (__GNUC__ > 11) or (__GNUC__ <= 11) which allows the CI tests to pass. I'm not entirely sure if the behavior changed with GCC 11, though, and haven't figured out exactly which version caused the change.
—
Reply to this email directly, view it on GitHub<https://urldefense.us/v3/__https://github.com/flux-framework/flux-sched/pull/989*issuecomment-1685418209__;Iw!!G2kpM7uM-TzIFchu!0KyC9OsSV4e6XnA5e_Mg0VfZqJUgpk4Y21WHWT_U4yOn6sOdTTrBn9pl5zmgN0Ia-cJ_jwMx4kKiBNcjdUaf198qTIE$>, or unsubscribe<https://urldefense.us/v3/__https://github.com/notifications/unsubscribe-auth/AAFBFNLAKV2YCTJAQZ2MA2TXWKJAFANCNFSM6AAAAAASQI7AXY__;!!G2kpM7uM-TzIFchu!0KyC9OsSV4e6XnA5e_Mg0VfZqJUgpk4Y21WHWT_U4yOn6sOdTTrBn9pl5zmgN0Ia-cJ_jwMx4kKiBNcjdUafaEUR2G8$>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Problem: testsuite tests will need to compare pool_infra_t via the `==` operator to check if copying, destruction, and assignment works as needed. Add the required member checks.
Problem: the testsuite needs to test if copying, destroying, and assigning planners works as desired. Namely, calling the copy constructor should deep copy planners, and assignment should ensure the two planner's states are independent but the same initially. Mutation of a copy should not affect the source object. Destruction should not affect the source object. Add low-level checks for planner behavior.
Problem: the testsuite needs to test if copying, destroying, and assigning planner_multis works as desired. Namely, calling the copy constructor should deep copy planner_multis, and assignment should ensure the two planner_multis' states are independent but the same initially. Mutation of a copy should not affect the source object. Destruction should not affect the source object. Add low-level checks for planner_multis behavior.
Add unit tests to ensure the copy constructor, assignment overload, and equality overload function as needed.
Add unit tests to ensure the copy constructor, assignment overload, and equality overload for pool_infra_t function as needed.
Add the schema tests to gitignore to ensure undesired files in the directory are not tracked by git.
Add tests for attaching a new subgraph to an instantiated resource graph and ensure the allocations and reservations are not affected. Test that the match policy selects appropriate resources from the newly added subgraph.
Problem: schema_test01 and schema_test02 have misspellings of "constructors" and "assignment." Fix the spelling.
882a80e
to
a8c3208
Compare
Thanks for your detailed and helpful feedback @trws. I think I've addressed and implemented your comments and suggestions. The PR is ready for another review. |
Problem: with GCC, Yggdrasil and Boost libraries generate compilation errors related to missing template keywords and maybe uninitialized values, respectively. Suppression flags for GCC are unrecognized by Clang and produce compilation errors. Add conditional diagnostic pragmas to suppress the errors for GCC but not Clang.
1b38382
to
8761d50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this, thanks @milroy! There's always refactoring I'd love to do, but I don't think it makes sense to do it before we bump the C++ version. Delegated constructors and some other things could help us with code like this in the future.
@trws should I set MWP, or do you think this needs a review from someone else as well? Maybe @tpatki @JaeseungYeom or @jameshcorbett could spot check a few of the commits. |
+1 with merging @trws @milroy -- we've reviewed it together, and looked at it a few times (I looked at it in Feb as well). The changes make sense to me and seem correct overall. I haven't had a chance to test it locally at my end, and I will do that soon -- but we don't need to hold on merging this for my testing. We can always open an issue if something comes up when I try it out. |
Thanks everyone! Setting MWP. |
I missed some mergify requirements when transferring this, or maybe updated them after.
@milroy, I just pushed up a CI config fix. This should let everything merge. |
Codecov Report
@@ Coverage Diff @@
## master #989 +/- ##
========================================
+ Coverage 72.6% 73.4% +0.8%
========================================
Files 80 98 +18
Lines 10243 11888 +1645
========================================
+ Hits 7437 8732 +1295
- Misses 2806 3156 +350
|
Enable Fluxion resource graph elasticity
This PR enables elasticity in the Fluxion resource graph by creating requisite ctors, dtors, and assignment overloads for
planner
, andplanner_multi
.As discussed in PR #775, attaching a new subgraph to the boost resource graph without preallocating sufficient vertices in the
vecS
associative Boost container triggers a copy. Sinceplanner
did not have a copy constructor to deep copy the red-black trees and related data strctures, the copy cleared the RBtrees and allocations and reservations.By defining copy constructors for these classes and data structures, subgraph
attach
and future subgraphshrink
will preserve allocations and reservations.This PR refactors
planner
andplanner_multi
from structs to classes. Operations on both classes (e.g., setters and getters) have been pulled into the class as member functions, and access controlled viaconst
member functions where appropriate.The PR is WIP, as there are still several tasks to complete which may require reviewer feedback:.h
and.hpp
) structure for compatibility with external C clients..h
and.hpp
) structure for compatibility with external C++ clients.planner_t
andplanner_multi_t
to avoid errors related to forward declarations.planner_multi
andpool_infra_t
ctors and assignment overload for elastic resources.I've changed the
planner
interface as well as the build system to accommodate C and C++ clients. I added unit tests for ctors and assignment overload operator forplanner_t
andplanner_multi_t
, and created new unit tests for ctors and assignment overload operator for schema (bothschedule_t
andpool_infra_t
).