HPP: refresh Machine class and MachineQueryIterator class - #336
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refreshes the Machine class and MachineQueryIterator class in the HPP interface. It implements previously unimplemented methods, refactors callback functions for better clarity, and adds comprehensive unit tests.
- Implements
Machine::get_machine()and iterator functionality forMachineQueryIterator - Refactors callback templates to be more explicit about handle types
- Improves error messages to include status codes
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit_tests/hpp_machine_tests.cc | Adds comprehensive unit tests for Machine class functionality including processor/memory queries |
| tests/CMakeLists.txt | Registers the new test file in the build system |
| src/realm.hpp | Implements Machine::get_machine(), refactors callback templates, implements MachineQueryIterator operators, and improves error messages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| // Verify all processors exist | ||
| for(const auto &proc : processors) { | ||
| EXPECT_TRUE(proc.exists()); |
There was a problem hiding this comment.
Should verify that all the processors returned match the specification (one gpu, which node they're on, etc). It's not enough to just iterate through the given list and ensure they all exist.
c5096c3 to
e2e9d9f
Compare
| // ============================================================================ | ||
|
|
||
| INSTANTIATE_TEST_SUITE_P(MachineModelVariations, HPPMachineTest, | ||
| ::testing::Values(createBasicSingleNodeConfig(), |
There was a problem hiding this comment.
Try taking a look at topology_test.cc for a slightly better way to lay this out.
| } | ||
| } | ||
|
|
||
| TEST_P(HPPMachineTest, QueryLocalProcessorsByCPUKind) |
There was a problem hiding this comment.
This can be made more data driven as well. For example, you can loop over all kinds and perform the query. Or you can pass in, as an extra test parameter, the kind to look for, taking to make sure to test either kinds that can't exist (like an invalid enum value) as well as ones that don't happen to exist in the current machine model (e.g. asking for gpus when there aren't any in the machine model). All of these should verify not only that the returned processors are valid and have the appropriate kind, but also return all the processors specified that meet the appropriate kind. Additionally, since this is a query for only the local processors, it should follow that we much also verify that all processors have the same address space as the one specified to be local in the machine config, and a test case needs to be made such that there are more than one address space / node to verify these are not returned. By doing this, you'll see that you incrementally improve coverage with each data test case without having to modify the actual logic of the test, making the test easily scale to all sorts of different corner cases.
There was a problem hiding this comment.
I've updated the test to do more with instantiated topologies
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EXPECT_EQ(proc.address_space(), 0) | ||
| << "Processor should be in local address space (0), but is in " | ||
| << proc.address_space(); | ||
| actualProcessorIds.insert(proc.id); |
There was a problem hiding this comment.
Inserting proc.id (the raw processor ID) instead of the processor index (proc_id.proc_proc_idx()) creates a mismatch with expectedProcessorIds which contains processor indices from the configuration. This will cause the comparison at line 504 to fail. Should use Realm::ID(proc.id).proc_proc_idx() to extract the processor index.
| actualProcessorIds.insert(proc.id); | |
| actualProcessorIds.insert(Realm::ID(proc.id).proc_proc_idx()); |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
==========================================
+ Coverage 27.74% 28.16% +0.42%
==========================================
Files 191 192 +1
Lines 39499 39805 +306
Branches 14382 14252 -130
==========================================
+ Hits 10960 11213 +253
+ Misses 28147 27145 -1002
- Partials 392 1447 +1055 ☔ View full report in Codecov by Sentry. |
No description provided.