Parallel run with one proc w/o partition file #721
Conversation
|
Thanks Phil for the review. Will work on it first thing in the morning
Monday.
…On Fri, Feb 23, 2024 at 4:58 PM Phil Miller - NOAA ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In include/core/Partition_Parser.hpp
<#721 (comment)>:
> @@ -63,8 +52,9 @@ class Partitions_Parser {
std::string remote_nex_id;
std::string remote_cat_id;
std::string direction;
- Tuple tmp_tuple;
- std::vector<Tuple> remote_conn_vec;
+ //Tuple tmp_tuple;
⬇️ Suggested change
- //Tuple tmp_tuple;
—
Reply to this email directly, view it on GitHub
<#721 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRPCKRGXENW67MF3EDDYVENI7AVCNFSM6AAAAABCW7KOJOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJZGA3DGMJTGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
test/CMakeLists.txt
Outdated
| NGen::realizations_catchment | ||
| NGen::mdarray | ||
| NGen::mdframe | ||
| NGen::logging | ||
| NGen::ngen_bmi | ||
| testbmicppmodel | ||
| REQUIRES | ||
| NGEN_WITH_SQLITE |
There was a problem hiding this comment.
I'm concerned about making the entire test_unit build dependent on having SQLite support enabled.
There was a problem hiding this comment.
Maybe the test case could say
#if !defined(NGEN_WITH_SQLITE3)
GTEST_SKIP() << "Compiled without SQLite support"
#endif
?
There was a problem hiding this comment.
The problem is with the geopackage is involved in the test code.
There was a problem hiding this comment.
Haven't reviewed this, but IIRC from an earlier discussion, you could take out the GeoPackage reading, and just use GeoJSON, since it isn't needed to test the functionality of this PR.
PhilMiller
force-pushed
the
ngen_i703
branch
from
2a45154
to
662b770
Compare
test/utils/Partition_One_Test.cpp
Outdated
| if( duplicates.size() > 0 ){ | ||
| for( auto& id: duplicates){ | ||
| } | ||
| exit(1); | ||
| } | ||
|
|
||
| ASSERT_TRUE(true); |
There was a problem hiding this comment.
| if( duplicates.size() > 0 ){ | |
| for( auto& id: duplicates){ | |
| } | |
| exit(1); | |
| } | |
| ASSERT_TRUE(true); | |
| for( auto& id: duplicates){ | |
| // Add printing code as needed | |
| } | |
| ASSERT_EQ(duplicates.size(), 0); |
Unit tests should never be calling exit() directly, since that kills the whole test process, and doesn't properly report the failed test result.
The loop over duplicates doesn't need to be conditional, since if there are none, it does nothing.
Same applies to 1b below.
|
I pushed fixes for some of the smaller things I noted in my review. I wanted to call my concerns about the test code to your attention. |
|
Could you be more explicit?
…On Fri, Feb 23, 2024 at 7:24 PM Phil Miller - NOAA ***@***.***> wrote:
I pushed fixes for some of the smaller things I noted in my review. I
wanted to call my concerns about the test code to your attention.
—
Reply to this email directly, view it on GitHub
<#721 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRIH4WC4N32BDV72AKLYVE6NBAVCNFSM6AAAAABCW7KOJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRSGIYDAOJUGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Ignore my previous question if you don't have any more to add. Just read your comments on the test codes. |
|
I just put a couple of comments in the PR page.
On Mon, Feb 26, 2024 at 7:19 AM Shengting Cui - NOAA Affiliate <
***@***.***> wrote:
… Could you be more explicit?
On Fri, Feb 23, 2024 at 7:24 PM Phil Miller - NOAA <
***@***.***> wrote:
> I pushed fixes for some of the smaller things I noted in my review. I
> wanted to call my concerns about the test code to your attention.
>
> —
> Reply to this email directly, view it on GitHub
> <#721 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACA4SRIH4WC4N32BDV72AKLYVE6NBAVCNFSM6AAAAABCW7KOJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRSGIYDAOJUGM>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
|
I have made an update by making Partition_One an independent test in
analogy with some other packages.
…On Mon, Feb 26, 2024 at 1:51 PM Justin Singh-M. - NOAA < ***@***.***> wrote:
Assigned #721 <#721> to @stcui007
<https://github.com/stcui007>.
—
Reply to this email directly, view it on GitHub
<#721 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRLRFX4BPAFTI56NIPLYVTRSXAVCNFSM6AAAAABCW7KOJOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJRHEZDMOBSGUZTGMA>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
include/core/Partition_One.hpp
Outdated
| #ifndef PARTITION_ONE_H | ||
| #define PARTITION_ONE_H | ||
|
|
||
| #ifdef NGEN_MPI_ACTIVE |
There was a problem hiding this comment.
The guard for NGEN_MPI_ACTIVE isn't necessary here. MPI's not actually referenced or used by any of this code. Removing the guard would simplify test code by removing the weird #define NGEN_MPI_ACTIVE
There was a problem hiding this comment.
The presence of NGEN_MPI_ACTIVE does make the code look a bit cumbersome. However, Partition_One is specifically designed for this case. The source codes are under this condition. On the other hand, I assume if the test code works w/o MPI. it is probably ok.
There was a problem hiding this comment.
The NGEN_MPI_ACTIVE is needed as Partition_One.hpp needs it. Without it, the test code won't compile. The Parttion_Test.cpp using Partition_Parser.hpp works in a similar fashion. So this is kept in place.
There was a problem hiding this comment.
Nothing in this class Partition_One actually uses MPI, so we don't need to guard the class definition for MPI availability.
If we don't guard the class, then we don't need to do an inappropriate #define in the test.
|
Thanks for the careful review. Will work on it.
…On Fri, Mar 8, 2024 at 7:13 PM Phil Miller - NOAA ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/utils/Partition_One_Test.cpp
<#721 (comment)>:
> + hydro_fabric_paths = {
+ "data/",
+ "./data/",
+ "../data/",
+ "../../data/",
+
+ };
+}
+
+TEST_F(PartitionOneTest, TestPartitionData_1a)
+{
+ read_file_generate_partition_data();
+
+ partition_one.generate_partition(catchment_collection);
+ PartitionData data_struct = partition_one.partition_data;
+ catchment_ids = data_struct.catchment_ids;
I just realized an oversight in these tests - we also need to check that
catchment_ids and nexus_ids contain the right number of elements.
Also, I just noticed that the comparison isn't the partition output
against the original catchment_collection, but just against itself. The
test as a whole should affirm that the set in the single partition is
identical to the set in the input it was given.
—
Reply to this email directly, view it on GitHub
<#721 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRJD7XR3UL2RUQ23IG3YXJO3ZAVCNFSM6AAAAABCW7KOJOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMRWGA2TOMBTGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Code looks good. I'm going to wait until we've got #767 merged to rebase and merge this. |
…implicit single-process partition
This PR address issue #703 in ngen framework, i.e., to run a MPI job using one processor without providing a partition file. For more than one processor, you must provide a partition file. If you insist on providing the partition file in the case of one processor, you are still ok.
Additions
PartitionOnefor to present a whole-hydrofabric partition for a single process, and associated testRemovals
None
Changes
PartitionOnewhen running on a single MPI process, rather than expecting a partition file argument on the command lineTesting
Passed all local test except RoutingPyBindTest.TestRoutingPyBind
Passing all automated testing
Screenshots
Notes
Command line example to run a job without/with partition file:
Checklist