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

MPI: check presence of a runner command only with run and test apps #937

Merged
merged 13 commits into from
Jun 27, 2023

Conversation

perazz
Copy link
Contributor

@perazz perazz commented Jun 14, 2023

Address #935.

  • if MPI can’t find a runner, do not stop if we are not running either fpm run or test
  • with MPI, --runner option overrides mpiexec instead of appending it (backward compatibility)
  • add --runner-args option so additional arguments can be passed to the runner command instead of to the app (e.g., "-np 123"

@Carltoffel all CI tests work as there is always a runner command there.
Can I ask you to test this in the case you detailed in #935?

Thank you!

@Carltoffel
Copy link
Member

Thanks for fixing this issue!
While it works, it stills prints very verbose warnings:

 + which mpiexec
/usr/bin/which: no mpiexec in (...)
+ which mpirun
/usr/bin/which: no mpirun in (...)
 + which mpiexec.exe
/usr/bin/which: no mpiexec.exe in (...)
 + which mpirun.exe
/usr/bin/which: no mpirun.exe in (...)

Where (...) is my whole path environment.

After printing these warnings, it starts compiling.

@perazz
Copy link
Contributor Author

perazz commented Jun 14, 2023

Good catch thank you @Carltoffel, the unnecessary messages should be gone now (only output on --verbose)

@Carltoffel
Copy link
Member

Another thing and sorry for the confusion I may have caused: I just updated the issue because fpm test doesn't work by simply providing a runner via --runner. Maybe it is possible to add another check, if a runner was provided and only if not search for mpiexec and mpirun?

Copy link
Member

@henilp105 henilp105 left a comment

Choose a reason for hiding this comment

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

Thanks @perazz , looks good to me.

@Carltoffel
Copy link
Member

Good catch thank you @Carltoffel, the unnecessary messages should be gone now (only output on --verbose)

Much better!
Now it produces this output:

 + which mpiexec
 + which mpirun
 + which mpiexec.exe
 + which mpirun.exe

@perazz
Copy link
Contributor Author

perazz commented Jun 14, 2023

The current logic is to pass only additional arguments with the runner flag, like:

./fpm run --runner=" -np 4"

queue systems have rather convoluted syntax and there is no straightforward way to support them via fpm run unfortunately.

@Carltoffel
Copy link
Member

Carltoffel commented Jun 14, 2023

queue systems have rather convoluted syntax and there is no straightforward way to support them via fpm run unfortunately.

I previously did it like this:

fpm run --runner "sbatch juwels.sh"

juwels.sh:

#!/usr/bin/ksh
#SBATCH --time=00:01:00
#SBATCH --nodes=1
#SBATCH --ntasks=4
#SBATCH --cpus-per-task=1

time srun $1

And it worked fine.

Edit: $1 is the executable name provided by fpm.

@Carltoffel
Copy link
Member

The current logic is to pass only additional arguments with the runner flag, like:

Is it like that only when using MPI? In this case I would suggest using another option-name as it is inconsistent with how it worked before or without MPI.

@perazz
Copy link
Contributor Author

perazz commented Jun 14, 2023

I need to check the code again and get back to you.
The reason it is this way is that there was a CLI issue in the passing of -np N or -np * via fpm's

  -- ARGS    Arguments to pass to executables.

@Carltoffel
Copy link
Member

Just an idea, what do you think about a new fpm subcommand: fpm mpirun?

src/fpm_meta.f90 Outdated Show resolved Hide resolved
src/fpm_meta.f90 Outdated Show resolved Hide resolved
perazz and others added 3 commits June 14, 2023 09:15
Co-authored-by: Minh Dao <43783196+minhqdao@users.noreply.github.com>
Comment on lines -1008 to +1019
this%run_command = mpi_wrapper_query(mpilib,fort_wrapper,'runner',verbose,error)
if (allocated(error)) return
this%has_run_command = len_trim(this%run_command)>0
this%run_command = mpi_wrapper_query(mpilib,fort_wrapper,'runner',verbose,runner_error)
this%has_run_command = (len_trim(this%run_command)>0) .and. .not.allocated(runner_error)
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines before the change look pretty good to me. What speaks against them? We return early if there's an error and therefore do not need to care about the value of this%has_run_command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were changed to account for your request @minhqdao. If you think the previous version was better, please revert, thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the question was what speaks against the lines 1008-1010 (before this PR)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirm my previous answer

@perazz
Copy link
Contributor Author

perazz commented Jun 15, 2023

Just an idea, what do you think about a new fpm subcommand: fpm mpirun?

@Carltoffel I checked and can confirm that there is currently no way to pass flags other than via --runner "my_runner -f1 -g2 -h3"
I am not sure if there would be consensus on adding another (different) runner command, but maybe we can split the two of them? Something like:

fpm run --runner="my_script.sh" --runner-flags=" -np 2 -N 10"

In this way, --runner would override the default MPI runner instead of cating it, which makes more sense

@Carltoffel
Copy link
Member

fpm run --runner="my_script.sh" --runner-flags=" -np 2 -N 10"

I really like this idea!

But then we should check if the argument of --runner is an executable without any flags.

@perazz
Copy link
Contributor Author

perazz commented Jun 15, 2023

Thanks @Carltoffel, I think that would be implicitly taken care of:

The rule would be,

  • --runner defines a running command, overriding any default ones (sh, cmd.exe, or mpiexec if MPI). It may or may not include additional arguments
  • --runner-args specify additional arguments (better args than flags) that will be appended to the runner command anyways.

I think this would be a pretty general approach?

@Carltoffel
Copy link
Member

Carltoffel commented Jun 15, 2023

It may or may not include additional arguments

I'm not sure, if we should forbid passing arguments to the runner. At least we should print a warning, IMHO.

Edit: Besides of this, I totally agree with you.

@perazz
Copy link
Contributor Author

perazz commented Jun 16, 2023

If we look at the fpm help, it says:

  --runner CMD   Provides a command to prefix program execution paths.
  -- ARGS    Arguments to pass to executables.

I've tried to pass the ARGS in every possible way but they don't seem to work when a runner is present. Probably a feature that was initially foreseen but never fully implemented. So I think at the end of the day, it's probably not too bad if we give it a name and call it

  --runner-flags ARGS    Arguments to pass to executables.

@perazz
Copy link
Contributor Author

perazz commented Jun 17, 2023

@Carltoffel like discussed, I've introduced the option to pass arguments to the runner (instead of to the app) via --runner-args.

So for example, one can now pass arguments to the runner e.g. mpiexec via:

./fpm run --runner-args=" -np 2"

(note heading space, needed by M_CLI2).
In presence of MPI, the runner command is also not appended anymore but mpiexec is only used if there is no user-provided runner (which restores backward compatibility).

@urbanjost I've tried to add a CI test but I keep failing writing "namelist language". If it is not too much of an issue, can I ask you to take a look at this PR? Thank you in advance.

@perazz
Copy link
Contributor Author

perazz commented Jun 21, 2023

I believe the comments and concerns so far were addressed; so, if there are no further ones, I think the content of this PR is ready to merge soon

@urbanjost
Copy link
Contributor

missed this one. Not quite sure what the exact issue is; but there was an earlier version prototype proposed where you could control the position in the command of the filenames by using a special string like %% or {} or $FILE. If the special string (lets say it is %% is found is is replaced much like commands like xargs or find do. xargs is particular seems a good model, as the -I switch lets you specify a string to replace, and that triggers the replacement operation. Would that be a useful solution? There are several string replacement procedures available.

@perazz
Copy link
Contributor Author

perazz commented Jun 22, 2023

no problem @urbanjost, the request is to be able to pass arguments to the runner command rather than the project executable. MPI is the example that triggered this need, as one has:

[/path/to/mpiexec] [-np 123 -N 456] [/path/to/project/.exe] [ARGS]

Now, to pass [-np 123 -N 456] one should have used --runner, but in this case, the path to the mpiexec would get lost (this is something the MPI metapackage searches for). So now --runner-flags allows to append flags to the runner executable. This is also backward compatible, as the former option is still valid

@perazz
Copy link
Contributor Author

perazz commented Jun 24, 2023

If there are no more comments, I think this can be merged soon.

@perazz perazz merged commit 3f511db into fortran-lang:main Jun 27, 2023
15 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.

None yet

5 participants