Skip to content

chore: Remove MPI - #2192

Merged
trisyoungs merged 34 commits into
develop2from
dissolve2/remove-mpi
Jul 14, 2025
Merged

chore: Remove MPI#2192
trisyoungs merged 34 commits into
develop2from
dissolve2/remove-mpi

Conversation

@trisyoungs

@trisyoungs trisyoungs commented Jul 2, 2025

Copy link
Copy Markdown
Member

I don't really know how you go about reviewing this, but this PR removes all trace of MPI-related parallelism from the code. The various Distributor classes as well as ChangeStore are now basically redundant, but the effort to remove usage of them is left for when relevant modules are converted to Nodes.

TODO

  • Check usage of Distributor and associated classes
  • ChangeStore
  • ModuleContext
  • Sweep for #ifdef PARALLEL and Parallel Comms
  • Sweep for commsTimer
  • Benchmarks
  • Test for procedures??
  • nix

Closes #562.

@trisyoungs
trisyoungs force-pushed the dissolve2/remove-mpi branch from 31f7c58 to 7c92728 Compare July 3, 2025 10:01

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 4342376 Previous: 467a003 Ratio
BM_Box_MinimumDistance<OrthorhombicBox> 12.91046146999677 ns/iter 5.29815982374698 ns/iter 2.44
BM_Box_MinimumDistance<MonoclinicAlphaBox> 11.481027478519398 ns/iter 5.6044062930106175 ns/iter 2.05
BM_Box_MinimumDistanceSquared<MonoclinicAlphaBox> 11.956455172902752 ns/iter 4.977398883100473 ns/iter 2.40

This comment was automatically generated by workflow using github-action-benchmark.

CC: @disorderedmaterials/dissolve-devs

@trisyoungs
trisyoungs force-pushed the dissolve2/remove-mpi branch from 7c92728 to d3e2857 Compare July 3, 2025 12:40
@trisyoungs
trisyoungs force-pushed the dissolve2/remove-mpi branch from d3e2857 to d17e057 Compare July 3, 2025 15:25
@trisyoungs
trisyoungs marked this pull request as ready for review July 4, 2025 13:25
@trisyoungs
trisyoungs requested a review from rprospero July 9, 2025 08:36

@rprospero rprospero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed out a PR that should fix the nix flake. You covered most of it - just needed to delete a few more lines we didn't need any more.

@rprospero

Copy link
Copy Markdown
Contributor

As I'm reviewing this, it looks like we can get rid of the chop_range method and the terribly named tests/algorthims/aragorn.cpp, as well as two comments in the gr node warning note to use chop_range there.

@rprospero rprospero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks much cleaner than before. I've made some comments above on extra code that we could delete, but that might just need to be a new issue. I've already added a couple of other issues from items I noticed during this review.

Comment thread src/nodes/gr/helpers.cpp
Comment on lines -408 to +393
for (auto it = cfg->molecules().begin() + offset; it < cfg->molecules().end(); it += nChunks)
for (auto &mol : cfg->molecules())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This single change was worth reviewing 233 files.

@trisyoungs

Copy link
Copy Markdown
Member Author

As I'm reviewing this, it looks like we can get rid of the chop_range method and the terribly named tests/algorthims/aragorn.cpp, as well as two comments in the gr node warning note to use chop_range there.

Now removed in this PR.

@trisyoungs
trisyoungs merged commit bf8cff4 into develop2 Jul 14, 2025
9 of 10 checks passed
@trisyoungs
trisyoungs deleted the dissolve2/remove-mpi branch July 14, 2025 08:18
rprospero added a commit that referenced this pull request Sep 5, 2025
Co-authored-by: Tristan Youngs <trisyoungs@googlemail.com>
Co-authored-by: Adam Washington <adam.washington@stfc.ac.uk>
rprospero added a commit that referenced this pull request Feb 4, 2026
Co-authored-by: Tristan Youngs <trisyoungs@googlemail.com>
Co-authored-by: Adam Washington <adam.washington@stfc.ac.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants