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

Default to add '-wdir' arguments #351

Merged
merged 4 commits into from
Aug 2, 2023

Conversation

jo-basevi
Copy link
Collaborator

@jo-basevi jo-basevi commented Jul 31, 2023

Change logic in experiments to default to adding -wdir to the arguments given to mpirun.
Should close #341 and close #350

@jo-basevi jo-basevi self-assigned this Jul 31, 2023
@coveralls
Copy link

coveralls commented Jul 31, 2023

Coverage Status

coverage: 41.798% (+0.5%) from 41.267% when pulling f044039 on jo-basevi:341-missing-wdir-arg into aacfd92 on payu-org:master.

@jo-basevi
Copy link
Collaborator Author

Is the MVIPICH wrapper still used? If so, I could revert the changes and only add ‘-wdir’ arguments if mpi_module does not start with ‘mvapich’ rather than previously which was checking mpi_module starts with openmpi?

@jo-basevi jo-basevi marked this pull request as ready for review July 31, 2023 01:03
@jo-basevi jo-basevi marked this pull request as draft July 31, 2023 02:05
@aidanheerdegen
Copy link
Collaborator

Looks like MPICH supports -wdir in any case, so this code is redundant and should be removed

https://www.mpich.org/static/docs/v3.4.x/www1/mpiexec.html

See #350

payu/envmod.py Show resolved Hide resolved
@jo-basevi jo-basevi marked this pull request as ready for review July 31, 2023 06:34
Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

More feedback!

payu/fsops.py Outdated Show resolved Hide resolved
payu/fsops.py Outdated
Comment on lines 189 to 201
# NOTE: ldd <binary>

# Example 1:
# libmpi.so.40 => /apps/openmpi/4.0.2/lib/libmpi.so.40 (0x00007fa493665000)
# libmpi_usempif08_Intel.so.40 => /apps/openmpi/4.0.2/lib/libmpi_usempif08_Intel.so.40 (0x00007fa492a7c000)
# libmpi_usempi_ignore_tkr_Intel.so.40 => /apps/openmpi/4.0.2/lib/libmpi_usempi_ignore_tkr_Intel.so.40 (0x00007fa492863000)
# libmpi_mpifh_Intel.so.40 => /apps/openmpi/4.0.2/lib/libmpi_mpifh_Intel.so.40 (0x00007fa4925cb000)

# Example 2:
# libmpi_usempif08.so.40 => $SPACKDIR/opt/spack/linux-rocky8-cascadelake/intel-2019.5.281/openmpi-4.1.5-ooyg5wc7sa3tvmcpazqqb44pzip3wbyo/lib/libmpi_usempif08.so.40 (0x00007f12671c0000)
# libmpi_usempi_ignore_tkr.so.40 => $SPACKDIR/opt/spack/linux-rocky8-cascadelake/intel-2019.5.281/openmpi-4.1.5-ooyg5wc7sa3tvmcpazqqb44pzip3wbyo/lib/libmpi_usempi_ignore_tkr.so.40 (0x00007f1266fb4000)
# libmpi_mpifh.so.40 => $SPACKDIR/opt/spack/linux-rocky8-cascadelake/intel-2019.5.281/openmpi-4.1.5-ooyg5wc7sa3tvmcpazqqb44pzip3wbyo/lib/libmpi_mpifh.so.40 (0x00007f1266d44000)
# libmpi.so.40 => $SPACKDIR/opt/spack/linux-rocky8-cascadelake/intel-2019.5.281/openmpi-4.1.5-ooyg5wc7sa3tvmcpazqqb44pzip3wbyo/lib/libmpi.so.40 (0x00007f12667c3000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my opinion these examples should be moved into a test of required_libs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I've separating the parsing of ldd output into a separate file so its simpler to test. The other option could be to mock out the ldd call?

payu/envmod.py Show resolved Hide resolved
payu/experiment.py Show resolved Hide resolved
Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

Agreed you could mock ldd as an alternative to storing the output in a file, but seems a lot of hassle and I kind of like the resulting design as it isolates functionality, which is not only good for your testing, but allows someone else to call a function like parse_ldd_output independently of running payu, which can help with checking for errors, or ensuring the code is doing what you expect.

Comment on lines -593 to -599
# Our MVAPICH wrapper does not support working directories
if mpi_module.startswith('mvapich'):
curdir = os.getcwd()
os.chdir(self.work_path)
else:
curdir = None

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wanted to leave a comment to say deleting this relates directly to #350.

This just can't work with submodels and there is no attempt to test for that, nor do we want to support something that is so limited, so it is being removed.

payu/experiment.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@aidanheerdegen aidanheerdegen left a comment

Choose a reason for hiding this comment

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

Feel free to merge.

@jo-basevi jo-basevi merged commit c8e7424 into payu-org:master Aug 2, 2023
5 checks passed
jo-basevi added a commit to jo-basevi/payu that referenced this pull request Aug 2, 2023
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.

MVAPICH run option incompatible with submodels missing -wdir arguments
3 participants