Refactor parallel.py (NGWPC-10583) - #201
Conversation
37c4ca9 to
d9d50cc
Compare
mxkpp
left a comment
mxkpp
left a comment
There was a problem hiding this comment.
This looks good, I just have a few minor comments, a question about reliability of the cleanup step, and I would like to test the switch from Bcast to bcast on a VPU-scale realization to confirm that we are not introducing a performance bottleneck when scaling.
| def broadcast_parameter(self, value_broadcast: T) -> T: | ||
| """Broadcast a single parameter value to all processors. | ||
|
|
||
| Generic function for sending a parameter value out to the processors. | ||
| :param value_broadcast: | ||
| :param config_options: | ||
| :return: | ||
| """ | ||
| dtype = np.dtype(param_type) | ||
|
|
||
| if self.rank == 0: | ||
| param = np.asarray(value_broadcast, dtype=dtype) | ||
| else: | ||
| param = np.empty(dtype=dtype, shape=()) | ||
|
|
||
| if self.size == 1: | ||
| return value_broadcast | ||
| try: | ||
| self.comm.Bcast(param, root=0) | ||
| except MPI.Exception: | ||
| config_options.errMsg = "Unable to broadcast single value from rank 0." | ||
| err_handler.log_critical(config_options, self) | ||
| return None | ||
| return param.item(0) | ||
|
|
||
| def scatter_array_logan(self, geoMeta, array_broadcast, ConfigOptions): | ||
| """Scatter an array based on the input dataset type. | ||
|
|
||
| Generic function for calling scatter functons based on | ||
| return self.comm.bcast(value_broadcast, root=0) | ||
| except Exception as e: | ||
| self.config_options.errMsg = f"Unable to broadcst single value {value_broadcast} from rank 0: {e.__class__.__name__} -- {e}" | ||
| err_handler.log_critical(self.config_options, self) | ||
| raise | ||
|
|
There was a problem hiding this comment.
This reads a lot more clearly than the original code that leveraged Bcast instead of bcast, but I wonder if some use cases do have large enough broadcast calls that the former would be noticeably faster. I don't think we'd notice when running small realizations such as individual USGS stream gage basins. But I want to test on a full-VPU realization, running with a profiler, to see the impact in that situation before we merge this.
|
Note: the pytest tests are passing after rebasing against development. |
mxkpp
left a comment
mxkpp
left a comment
There was a problem hiding this comment.
Looks good. Tested well. I just updated one commented-out block that was still referencing a symbol that had been renamed.
11795b1 to
ce2b581
Compare
|
Rebased |
ce2b581 to
9e8521e
Compare
|
Rebased against |
|
Tests are passing from RTE: ( cd src/ngen-forcing && pytest )
( cd src/ngen-forcing && mpirun -n 2 pytest ) |
2 participants
Rework parallel.py to add appropriate documentation and clean the processing.
Primary changes include remove the need to pass a
ConfigOptionsobject to the methods since an instance ofConfigOptionswill be saved on__init__.Additionally, the cleanup is no longer being tied to
atexitand instead needs to be explicitly called when exiting the program. The workflow of NGEN will almost always call the BMI'sfinalizemethod, so including cleanup in there should let normal runs of NGEN continue cleaning temporary files properly whilst removing the messyatexitdependency.Additions
Removals
Changes
Testing
Screenshots
Notes
Todos
Checklist
Testing checklist
Target Environment support