Repository navigation
Conversation
Purely additive: no call site changes, inheritance still in place. para_2d() (const + non-const) returns *this while Parallel_Orbitals derives from Parallel_2D, giving the two escape hatches a const getter cannot serve (LR_Util::setup_2d_division mutation, test-side descriptor writes). get_blacs_ctxt() completes the inherited getter family, which covers the other seven public data members.
…rate module_rt call sites
|
Thanks for slicing this refactor into a reviewable first step. Making the block-cyclic layout access explicit is the right direction, and keeping the change behavior-neutral while inheritance is retained is the correct sequencing. I verified the getter/member mapping (nloc -> get_local_size(), nrow -> get_row_size(), desc -> get_desc(), etc.) is one-to-one, and the test-side pv.set(...) -> pv.para_2d().set(...) substitutions are safe because Parallel_Orbitals does not override those methods. The non-cuBLASMp path is behavior-neutral. Two things I'd like addressed before this merges.
This repo squash-merges, so the PR title becomes the permanent commit subject on develop. The current title, "replace Parallel_2D inheritance with composition", records a change this PR does not make (: public Parallel_2D is untouched, as the body itself states). After merge, git log will point people looking for the inheritance removal to a commit that did not remove it. Suggested title: plaintext refactor(rt): make block-cyclic layout access explicit via para_2d() The body says "all module_rt consumers", but every #ifdef __CUBLASMP block still uses pv->nloc / pv->desc[i] / pv->dim0 directly: band_energy.cpp 133-349 Please either migrate them here (it is the same mechanical substitution) or explicitly scope them out in the body and list these six ranges in the follow-up plan. A few non-blocking suggestions: Naming: para_2d is introduced here with zero existing callers, so it can be given its final name (get_block_cyclic_distribution()) for free now. Renaming later costs another 24-file diff. In norm_psi.cpp / middle_hamilt.cpp print_matrix blocks, the column-major stride uses get_col_size() where get_row_size() is expected; invisible under 1x1 grids. |
|
@MrLi000001 thanks for your suggestion. I have modified the title of this PR. For the cusolvermp side, my idea coincides mostly with your second suggested solution, that is, scoping it out. I will make my final decisions on this after a closer look and evaluating the radius of the explosion from the code changes. Regards, |
|
This is a very challenging and thankless task. I've thought about doing this myself, but the historical baggage of the ABACUS codebase has always been too heavy. I believe this would be a major step forward if the 2D block-cyclic distribution and its related logic could be handled in a cleaner and simpler way. (After all, I partly gave up on developing LCAO algorithms because of all this spaghetti code :( especially the 2D block-cyclic stuff) As for the multi-GPU cuBLASMp/cuSOLVERMp path, that code was written in a rush and really should have been moved into a higher-level abstraction. Ideally, it should be unified with ScaLAPACK, since their programming models are essentially the same.🤔 (For example, we already have a unified abstraction layer with backend dispatch for single-process-CPU/single-GPU linear algebra libraries such as BLAS/LAPACK/cuBLAS/cuSOLVER) This would also make it easier to integrate other libraries like LibDDLA in the future. Anyway, thanks for taking on all this work! |
|
Oh and as for the multi-GPU tests, they can be triggered (hopefully) by /abacus-ci gpu |
|
/abacus-ci gpu |
GPU validation: failureGPU cases: 97 passed, 2 failed, 1 infrastructure. Open the Actions run | Download raw test files Candidate: |
I checked the logs and reran the tests on SAI. The two PW failures are pre-existing Max_wfc_* comparison failures; scf_out_wf matches #8153 exactly. The infrastructure failure comes from the current CI configuration requesting 10_NO_MD_boundary_shift, which is absent from this PR’s older source tree. Running the PR executable against the latest test cases gives 95 passed, 5 failed, 0 infrastructure: the additional RT failures exercise fixes from #8207 and #8229 that this branch does not yet contain. All three RT failures also reproduce on the PR’s merge base with essentially identical results, so they were not introduced by this refactor. Updating the branch to current develop should bring the code and tests back into alignment. For RTD, the notification explicitly reports a 900-second timeout, with the log ending at the ABACUS compilation step. |
|
@Stardust0831 Okay I see, thanks for clarification. Btw, are you the only person who have permission on approving GitHub deployment?[doge] I found that I can't review my own deployment request🤔 |
The current reviewers for gpu-ci-manual are mohanchen, Critsium-xy, and me. Self-review is allowed, but your account is not on the reviewer list yet. I think adding you to the list would make sense. I’m also planning to further improve code isolation so GPU validation can become an optional check in the regular CI workflow. An update on validation: I tested an isolated merge of this PR with the latest develop on SAI using the current test suite. The result is 98 passed, 2 failed, and 0 infrastructure errors. All 17 RT cases passed, and the two-node, 8-GPU cuSOLVERMp run completed successfully. The remaining two failures are the same PW Max_wfc failures seen in the develop nightly, including the known case documented in #8153. Based on these results, I think we can consider the current refactor to have passed multi-GPU parallel testing |
|
@AsTonyshment hi, thanks for your information, your suggestion is quite useful for me when handling the cusolvermp related codes. |


Summary
Make all
module_rtconsumers request their block-cyclic matrix layoutexplicitly through
Parallel_Orbitals::para_2d()instead of relying oninherited
Parallel_2Dmembers and implicit upcasts.This is the first independently mergeable slice of the longer refactor that
will replace
Parallel_2Dinheritance with composition and, later, introduceMatrix2das the boundary for LAPACK/ScaLAPACK operations. Inheritance isintentionally retained in this PR.
Motivation
Parallel_Orbitalscurrently exposes the layout of its baseParallel_2Ddirectly. RT code can therefore access
nloc,desc,dim0,dim1, andother distribution details without making the dependency visible.
That creates two problems:
pvrepresents basis, wavefunction, or matrix layout;This PR makes the dependency explicit and keeps the change behavior-neutral.
Changes
para_2d()accessors.get_blacs_ctxt()while the legacy getter family is still present.module_rtcall sites from inherited layout access topara_2d().Non-goals
Parallel_2Dinheritance.BlockCyclicDistributionInformationorMatrix2d.Those changes are kept for subsequent PRs so that each consumer migration can
be reviewed and verified independently.
Scope
Only the
module_rtcall sites and the small compatibility accessor inParallel_Orbitalsare changed.Verification
git diff --check upstream/develop...HEAD: passed.the only documentation warning is addressed by this no-docs-needed note.
Follow-up
para_2d()access.Parallel_2Dinheritance after all consumersare explicit.
BlockCyclicDistributionInformation.Matrix2d/Matrix2dViewas the math boundary.