Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Now the header is correct also in mpi run benchmark run #1103

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

Iximiel
Copy link
Member

@Iximiel Iximiel commented Jul 26, 2024

Description

Reading the code again, I discovered that shuffled and domain decomposition could be automatically changed without the user input, during the setup of the benchmark,
I changed the output of the header to reflect that.

Sorry for the Friday PR, this is a small thing that I am sure to forget

Target release

I would like my code to appear in release 2.10

Type of contribution
  • changes to code or doc authored by PLUMED developers, or additions of code in the core or within the default modules
  • changes to a module not authored by you
  • new module contribution or edit of a module authored by you
Copyright
  • I agree to transfer the copyright of the code I have written to the PLUMED developers or to the author of the code I am modifying.
  • the module I added or modified contains a COPYRIGHT file with the correct license information. Code should be released under an open source license. I also used the command cd src && ./header.sh mymodulename in order to make sure the headers of the module are correct.
Tests
  • I added a new regtest or modified an existing regtest to validate my changes.
  • I verified that all regtests are passed successfully on GitHub Actions.

@Iximiel Iximiel changed the title Now the header is correct also in mpi run simulation Now the header is correct also in mpi run benchmark run Jul 26, 2024
@Iximiel Iximiel force-pushed the benchmark-header-fix branch from c1e3931 to 154b442 Compare July 26, 2024 14:33
@GiovanniBussi
Copy link
Member

@Iximiel if I read the code correctly, this means that even if you run with --shuffled and without --domaindecomposition, the log will display running with --domaindecomposition. I think it is not correct. Did I misunderstand something?

@Iximiel
Copy link
Member Author

Iximiel commented Sep 3, 2024

In my intention should be the other way around, tomorrow I will check if is correct

maybe I should also add an 1 step benchmark regtest to enforce the beahviour of the header of the output

@Iximiel
Copy link
Member Author

Iximiel commented Sep 4, 2024

I checked out and compiled this branch:

mpirun -np2 plumed benchmark --natoms=2 --nsteps=1 --domain-decomposition starts with

BENCH:  Welcome to PLUMED benchmark
BENCH:  Using --kernel=this
BENCH:  Using --plumed=plumed.dat
BENCH:  Using --nsteps=1
BENCH:  Using --natoms=2
BENCH:  Using --maxtime=-1
BENCH:  Using --shuffled
BENCH:  Using --domain-decomposition
BENCH:  Using --sleep=0
BENCH:  Using --atom-distribution=line
BENCH:  Initializing the setup of the kernel(s)

plumed benchmark --natoms=2 --nsteps=1 --shuffled starts with:

BENCH:  Welcome to PLUMED benchmark
BENCH:  Using --kernel=this
BENCH:  Using --plumed=plumed.dat
BENCH:  Using --nsteps=1
BENCH:  Using --natoms=2
BENCH:  Using --maxtime=-1
BENCH:  Using --shuffled
BENCH:  Using --sleep=0
BENCH:  Using --atom-distribution=line
BENCH:  Initializing the setup of the kernel(s)

so shuffled does not activate the domain decomposition, but the dd activates shuffled, as intended


I also discovered this:
plumed --no-mpi benchmark --natoms=2 --nsteps=1 --domain-decomposition
echoes the first lines of the output then crashes, because it needs mpi function. But I think that the error message that appears is clear enough for an advanced user:

terminate called after throwing an instance of 'PLMD::Plumed::ExceptionError'
  what():  
(tools/Communicator.cpp:94) void PLMD::Communicator::Set_comm(const PLMD::TypesafePtr&)
+++ assertion failed: initialized()
you are trying to use an MPI function, but MPI is not initialized

@GiovanniBussi
Copy link
Member

so shuffled does not activate the domain decomposition, but the dd activates shuffled, as intended

This was my point. I mean, the log should not say --shuffled if the user didn't put it on the command line the --shuffled flag. It's ok to write Atoms are shuffled. But not --shuffle was used, if it was not...

@Iximiel
Copy link
Member Author

Iximiel commented Sep 5, 2024

When I set up this, my idea was to show the actual configuration.

I added this PR on top of the old one because I noted that having shuffle show on the output immediately after parsing did not show the configuration which the benchmark was running with.

On the other hand, with this approach, we have reproducible output even in case of a change of default parameters, so I show all the parameters even if the user did not specify them in the command line.

If I do a mpirun -np2 plumed benchmark --natoms=2 --nsteps=1 I get again:

BENCH:  Welcome to PLUMED benchmark
BENCH:  Using --kernel=this
BENCH:  Using --plumed=plumed.dat
BENCH:  Using --nsteps=1
BENCH:  Using --natoms=2
BENCH:  Using --maxtime=-1
BENCH:  Using --shuffled
BENCH:  Using --domain-decomposition
BENCH:  Using --sleep=0
BENCH:  Using --atom-distribution=line
BENCH:  Initializing the setup of the kernel(s)

Because it is hardcoded to use the dd in a mpi run and to use shuffled if the dd is on. On that I probably should add a note on the number of processes used and maybe also on the threads, if avaiable.

@carlocamilloni
Copy link
Member

@Iximiel @GiovanniBussi I think here the idea is to report the actual running configuration including both implicit and explicit choices, so if I would say it can be merged. I would retarget on 2.10 and report the number of process/threads as well

@Iximiel Iximiel changed the base branch from master to v2.10 October 10, 2024 13:11
@Iximiel
Copy link
Member Author

Iximiel commented Oct 10, 2024

@Iximiel @GiovanniBussi I think here the idea is to report the actual running configuration including both implicit and explicit choices

Yes

@carlocamilloni carlocamilloni merged commit 22c7106 into plumed:v2.10 Oct 11, 2024
22 checks passed
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.

3 participants