Refactor i-PI socket driver - #7939
Open
mohanchen wants to merge 13 commits into
Open
Conversation
added 6 commits
September 9, 2026 09:06
Move constants, structs, and helper functions from socket_driver.cpp's anonymous namespace into a dedicated socket_driver_utils module. This reduces socket_driver.cpp from 876 to 507 lines and improves its code quality score from 47 to 61. Update CMakeLists.txt and test/CMakeLists.txt to include the new source file in both the library and the socket_driver_test target.
Extract the five i-PI message branches (STATUS/INIT/POSDATA/GETFORCE/EXIT) from socket_driver.cpp into a dedicated socket_driver_handlers module. The main loop now only dispatches headers, reducing the socket_driver function's cyclomatic complexity from 50 to about 10 and raising its code quality score from 61 to 100. Introduce DriverContext to bundle shared mutable state (ESolver, UnitCell, Input_para, driver state, published frame, etc.) so each handler receives at most three parameters. Update CMakeLists.txt and test/CMakeLists.txt to include the new source file.
…utils Move Jacobi SVD and matrix utility functions from socket_frame.cpp's anonymous namespace into a dedicated socket_frame_utils module. This reduces socket_frame.cpp from 426 to 278 lines, making the remaining validation logic easier to decompose in follow-up changes. Update CMakeLists.txt and test/CMakeLists.txt to include the new source file in the library and both socket test targets.
…stages Split the validate_ipi_cell function into nine private helpers in an anonymous namespace: entry validation, tolerance validation, scale computation, determinant computation, SVD, condition number, inverse computation, and consistency check. The public function now reads as a linear pipeline, reducing its cyclomatic complexity from 27 to about 5 and raising socket_frame.cpp's code quality score from 72 to 89. Also change MATRIX_DIMENSION from an extern const to a constexpr in socket_frame_utils.h so it can be used as an array bound in the helper signatures.
…n helpers Decompose make_ipi_virial into three private helpers in an anonymous namespace: validate_virial_inputs, check_stress_symmetry, and compute_symmetric_virial. The public function now reads as a short pipeline, reducing its cyclomatic complexity from 16 to about 4 and raising socket_frame.cpp's code quality score from 89 to 95.
Rename all UPPER_CASE constants in the socket driver, frame, and IPI modules to kCamelCase per project style. Also split two long WARNING_QUIT/WARNING message strings in socket_driver_handlers.cpp. The remaining uppercase_constant deductions are from system macros (e.g., MPI_COMM_WORLD) and include guards in headers, which are not addressed by this change.
mohanchen
added
Refactor
…socket driver Replace direct MPI_Allreduce calls with Parallel_Reduce::reduce_min and reduce_max, and replace MPI_Bcast with Parallel_Common::bcast_int for int32 values. Add explicit instantiation of reduce_max<int> in parallel_reduce.cpp. Note: socket_driver_test currently fails in this build because the test binary links against an MPI-enabled base library but does not initialize MPI; this is a pre-existing test environment limitation.
Add socket_frame_utils.o, socket_driver_utils.o, and socket_driver_handlers.o to OBJS_RELAXATION so the Makefile build can link the new translation units introduced by the socket refactoring.
…I guards - Add custom main with MPI_Init/MPI_Finalize to socket_driver_test.cpp - Link MPI::MPI_CXX and KEEP_FEATURE_DEFINITIONS __MPI in test CMakeLists - Add Atom::bcast_atom/bcast_atom2 stubs in for_test.h to satisfy bcast_cell.cpp - Remove #ifdef __MPI guards from socket_driver_utils.cpp since test now initializes MPI and production binary always has MPI available
File renames: - socket_driver_utils → socket_utils - socket_driver_handlers → socket_handlers Namespace renames: - SocketDriverUtils → SocketUtils - SocketDriverHandlers → SocketHandlers - SocketFrameUtils → FrameUtils Function renames (remove redundant socket_ prefix): - bcast_socket_int → bcast_int - bcast_socket_int32 → bcast_int32 - bcast_socket_chars → bcast_chars - bcast_socket_string → bcast_string - socket_address → address - quit_if_root_io_failed → quit_if_root_failed
- socket_ipi_test.cpp → test_socket_ipi.cpp - socket_frame_test.cpp → test_socket_frame.cpp - socket_driver_test.cpp → test_socket_driver.cpp Follows AGENTS.md rule 11: new unit test files named test_<module_name>.cpp.
MPI_Comm_rank and MPI_Abort were called unconditionally in fail_during_collective_stage and is_root, breaking builds without __MPI defined. Wrap these calls (and the mpi.h include) with the standard __MPI guards. Non-MPI fallback behavior: - fail_during_collective_stage prints rank=-1 and aborts - is_root returns true (single process is always root)
Critsium-xy
reviewed
Sep 9, 2026
Critsium-xy
left a comment
Critsium-xy
left a comment
Collaborator
There was a problem hiding this comment.
Two issues found while checking the refactor for behavior preservation.
Comment on lines
+46
to
+63
| [[noreturn]] void fail_during_collective_stage(const char* stage, | ||
| const std::string& message) | ||
| { | ||
| int rank = -1; | ||
| #ifdef __MPI | ||
| MPI_Comm_rank(MPI_COMM_WORLD, &rank); | ||
| #endif | ||
| std::fprintf(stderr, | ||
| "ABACUS_SOCKET_MPI_FATAL stage=%s rank=%d message=%s\n", | ||
| stage, | ||
| rank, | ||
| message.c_str()); | ||
| std::fflush(stderr); | ||
| #ifdef __MPI | ||
| MPI_Abort(MPI_COMM_WORLD, EXIT_FAILURE); | ||
| #endif | ||
| std::abort(); | ||
| } |
Collaborator
There was a problem hiding this comment.
Behavior regression in serial builds.
Before this PR the #else branch (no __MPI) did throw std::runtime_error(message), which Socket_Driver::socket_driver catches and turns into a clean WARNING_QUIT with the reason written to warning.log. Here std::abort() is reached unconditionally, so an ESolver failure in an ENABLE_MPI=OFF build now SIGABRTs with no diagnostic in the log.
socket_driver is a user-facing INPUT switch and works in serial builds, so this is user-visible. CI cannot catch it: source/source_relax/CMakeLists.txt:32 guards add_subdirectory(test) with if(ENABLE_MPI), so these tests never build serially.
Suggest moving std::abort() back inside #ifdef __MPI and restoring throw std::runtime_error(message); in the #else branch.
Comment on lines
+431
to
+435
| if (is_root() && context.state != DriverState::Ready) | ||
| { | ||
| quit_if_root_failed(1, "POSDATA requires READY state"); | ||
| } | ||
| PosdataPayload payload = read_posdata(socket, *context.ucell); |
Collaborator
There was a problem hiding this comment.
This state check no longer sits on the collective path that every rank takes.
Previously the check set io_failed and all ranks went through one shared quit_if_root_io_failed. Now only the root enters this branch; non-root ranks fall straight into read_posdata() and reach its own quit_if_root_failed(0, "").
The two sides match only by coincidence — each happens to issue exactly one bcast_int plus one bcast_string. Any collective added to read_posdata() ahead of its quit_if_root_failed will deadlock.
Suggest passing context.state into read_posdata() and folding the check back into the existing io_failed / single-quit_if_root_failed pattern.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor i-PI socket driver