Mpi omp support #336

hiker · 2024-08-21T03:11:58Z

This PR adds configuration settings that enable/disable MPI and OpenMP to BuildConfig.

This means that an application can now specify that it should be compiled with OpenMP, without having to specify compiler-specific OpenMP flags - the Compiler object will get the information from the BuildConfig if it should add the flag or not.

Similarly, if an MPI build is requested, the tool repository will return an MPI enabled compiler (but the user can always overwrite this by hand-selecting a compiler explicitly).

Note that the MPI compiler wrapper added here will get a nicer (decorator-based) implementation in the next PR, I just tried to keep the PRs small. This PR is still quite big, but that's because I made the MPI and OpenMP settings mandatory (imho it makes sense to get the user to specify what exactly they want). The CLI defaults both to False. Additionally, I started to fix the line length (according to the discussion in #323

… already part of the config toolbox).

…_support

… happier.

…ccept a function that can return file-specific transformation scripts

…ork as expected.

Updates to compiler version checking

Add support for specifying MPI and OpenMP in the config file

hiker · 2024-08-28T02:16:18Z

Forgot to request a review sigh

MatthewHambley

The reformatting does add many extra changes but it needs to be done so I'm content with including it.

I think this change takes is in the right direction, my primary concern is the fact that tools for which "MPI" and "OpenMP" have no meaning have gained arguments for them.

Please give this some consideration. If tackling this issue would be a considerable amount of work I'm happy for you to raise an issue for it and link that to this change.

MatthewHambley · 2024-08-29T10:05:44Z

source/fab/build_config.py

+                 mpi: bool,
+                 openmp: bool,


I think there is an argument for making these optional with a default to False but that is not an argument we need to have now. It's always possible to make an interface more permissive.

As discussed in our meeting, I've made False the default, and updated tests and run_configs.

MatthewHambley · 2024-08-29T10:07:48Z

source/fab/build_config.py

    def __init__(self, project_label: str,
                 tool_box: ToolBox,
-                 multiprocessing: bool = True, n_procs: Optional[int] = None,
+                 mpi: bool,
+                 openmp: bool,
+                 multiprocessing: bool = True,
+                 n_procs: Optional[int] = None,
                 reuse_artefacts: bool = False,
-                 fab_workspace: Optional[Path] = None, two_stage=False,
-                 verbose=False):
+                 fab_workspace: Optional[Path] = None,
+                 two_stage: bool = False,
+                 verbose: bool = False):


A general observation that this argument list is getting pretty unwieldy. Something to consider at a future date.

Well, that seems unavoidable - in a complex build systems there are a lot of things to be defined :) Now that mpi/openmp are optional, it might be better? Also, this mostly will be handled by the command line tool in the end, which should have an appropriate man page/help command.

MatthewHambley · 2024-08-29T10:08:50Z

source/fab/build_config.py

+        :param mpi: whether the project uses MPI or not. This is used to
+            pick a default compiler (if not explicitly set in the ToolBox),
+            and controls PSyclone parameters.
+        :param openmp: whether the project should use OpenMP or not.


This argument presumably has the same caveats as the previous one. Is there a way to cover this without repeating boiler plate? Either group them and have some group documentation or with suitable wording.

No, it doesn't. We looked at all compiolers to be reasonably expected to be used (intel-classic, intel-llvm, gnu, clang, cray, nvidia) all of which support openmp.

MatthewHambley · 2024-08-29T10:32:05Z

source/fab/tools/tool_box.py

+            # TODO: Should we test if the compiler has MPI support if
+            # required? The original LFRic setup compiled files without
+            # MPI support (and used an mpi wrapper at link time), so for
+            # now we don't raise an exception here to ease porting - but
+            # we probably should raise one tbh.


Part of this would presumably also involve support for OpenMP. i.e. Adding an openmp argument to the method and ensuring the compiler supported it.

Same as above. I believe nowadays we can assume that compiler support openmp

MatthewHambley · 2024-08-29T10:36:11Z

source/fab/tools/tool_repository.py

+    def get_default(self, category: Category,
+                    mpi: Optional[bool] = None):
+        '''Returns the default tool for a given category. For most tools
+        that will be the first entry in the list of tools. The exception
+        are compilers and linker: in this case it must be specified if
+        MPI support is required or not. And the default return will be
+        the first tool that either supports MPI or not.


Your comment raises an important point. The concept of "MPI" makes no sense for most tools. We may need to rethink this whole API but maybe not for this change.

Can the MPI-ness be made part of the category? Something like tool_repo.get_default(FortranCompiler(mpi=True)).

@MatthewHambley
Fair point, I have half a dozen alternatives, here the few that I think are actually sensible:

We add separate get_compiler/get_linker methods (which take mpi as parameter), nothing else does.

We abort if a value for mpi is specified for any non-compiler/linker too. Or we turn this into kargs, meaning we can add tool-specific flags in the future, and for now only accept MPI for compiler/linker.

We add the BuildConfig as argument instead of mpi only. We can access the mpi status this way, and whatever any tool might need in the future could be done (admittedly I can't think of anything useful here :) ... maybe, and I am clutching at straws here, a tool could access the number of cores to use in a build, if a tool itself can run in parallel??)

Would one of these solution work for you? Or do you have a better idea?

@MatthewHambley That's the question I need your preference so that I can update this.

MatthewHambley · 2024-08-29T10:46:10Z

source/fab/steps/compile_fortran.py

+    compiler, flags_config = handle_compiler_args(config, common_flags,
+                                                  path_flags)


This is not using the MPI flag. Is that a problem? Is there a risk of a compiler other than the one returned with MPI flag being returned? If so the setup may not match the compiler which ends up being used.

The MPI flag is used when picking the compiler, so at this stage we already have an MPI-enabled compiler (if required).

MatthewHambley · 2024-08-29T10:54:16Z

source/fab/tools/compiler.py

+
+
+# ============================================================================
+class MpiGcc(Gcc):


I'm looking at this and I wonder if we need to more closely model the reality of the situation.

We can't simply have an "MPI compiler" which calls the "mpixxx" wrapper script because we need to pass command line arguments correct to the compiler being wrapped. But maybe we have an MPI compiler which takes (and wraps) a concrete compiler.

class MpiC(CCompiler): def __init__(self, compiler: CCompiler...

That was done in the next PR (I split this to make the individual PRs smaller and hopefully easier to review).
See e.g.
https://github.com/hiker/fab_new/blob/e5dd7c9dd0400b0f24d798539d50439a59fcb45e/source/fab/tools/compiler_wrapper.py#L31

MatthewHambley · 2024-08-29T11:00:06Z

tests/system_tests/git/test_git.py

+    return BuildConfig('proj', ToolBox(), mpi=False, openmp=False,
+                       fab_workspace=tmp_path)


This highlights an issue. Why does a test of Git functionality have to care about MPI and OpenMP?

Is that not another argument to having mpi and openmp optional arguments (that default to "False"?

hiker · 2024-09-02T09:30:45Z

The reformatting does add many extra changes but it needs to be done so I'm content with including it.

Yes. How did you do #323? Just running black with certain option? If you give us the option, we can do this between our upcoming PRs, to get it out of the way once. And I really appreciate not merging #323 in, as you can see, that will potentially cause us quite a bit of headache in terms of possible conflicts :(

MatthewHambley · 2024-09-19T12:29:28Z

Yes. How did you do #323? Just running black with certain option?

I did it by hand, directed by flake8.

hiker · 2024-10-21T03:08:36Z

OK, I'll just declare this ready for next review after giving mpi/openmp a default, to avoid the confusion about there his PR is at :)

hiker and others added 30 commits April 20, 2024 01:23

#3 Added get_hash function to compiler.

302d58b

#3 Added conftest.py file (with a fixture to create a C-compiler).

4068505

#3 Introduce fixtures for Fortran compiler and tool_box.

f24d939

#3 Remove explicit compiler information from MpCommonArgs (since it's…

075ed40

… already part of the config toolbox).

Merge branch 'master' into bom_master

15fca77

Merge branch 'bom_master' of github.com:hiker/fab into bom_master

d862762

Merge branch 'create_artefact_store' into add_standard_artefacts

880355a

Merge branch 'master' into 3_better_compiler_support

e999cd1

#3 Added linker as tool.

fdc402c

#3 Added test for linking shared libraries.

b50b3df

#3 Pass compiler flags to the linker if a compiler was specified.

b3806f1

#3 Remove unused function.

2cf63d3

#3 Removed more unused code.

2fffe56

#3 Automatically add a linker for each compiler.

7a8c293

#3 Fixed typo.

555de52

#3 Support vendor for compiler and linker.

abec46d

#3 Make linker having a vendor, too.

d4ad3f0

#3 Add set_default_vendor method to tool repository.

a64c9b7

Ignore build directory for git.

8a8781d

Merge branch 'master' into add_standard_artefacts

d967f7f

Updated test.

5472d7d

Merge remote-tracking branch 'upstream/master' into 3_better_compiler…

4c2f0d8

…_support

Merge remote-tracking branch 'upstream/master' into bom_master

ccbe839

# Fix some mypy errors and warnings.

227a153

Avoid using get() for singleton, instead use __new__ which makes mypy…

b8c64b9

… happier.

Changed the transformation_script parameter of function psyclone to a…

e309c15

…ccept a function that can return file-specific transformation scripts

Make mypy happy by using patch.object.

31a4877

Remove more comments and confusion about mypy :)

fbb15ea

Try to make mypy happy on older python versions.

7f52d40

Make flake8 happy.

ca58d67

hiker and others added 20 commits August 9, 2024 18:01

Added more tests for invalid version numbers.

9598c09

Added more test cases for invalid version number, improved regex to w…

5c6f99f

…ork as expected.

Fixed typo in test.

c1eb158

Merge branch 'version_tuples' into mpi_omp_support

1524777

Fixed test.

02fbb7b

Merge remote-tracking branch 'upstream/master' into bom_master

955befc

Merge pull request #18 from hiker/version_tuples

552e4cc

Updates to compiler version checking

Merge remote-tracking branch 'upstream/master' into mpi_omp_support

b38cdb8

Merge remote-tracking branch 'upstream/master' into version_tuples

6826c85

Merge branch 'version_tuples' into mpi_omp_support

e5aca3b

Split tests into smaller individual ones, fixed missing asssert in test.

ccbe810

Parameterised compiler version tests to also test wrapper.

f8f8ab0

Added missing MPI parameter when getting the compiler.

1543841

Fixed comments.

cd8ec62

Order parameters to be in same order for various compiler classes.

13935c0

Merge branch 'bom_master' into mpi_omp_support

de80c44

Merge remote-tracking branch 'origin/bom_master' into mpi_omp_support

009478f

Remove stray character

7887d24

Merge pull request #14 from hiker/mpi_omp_support

83836e7

Add support for specifying MPI and OpenMP in the config file

Merge remote-tracking branch 'MetOffice/master' into bom_master

a79b41d

hiker requested review from MatthewHambley and allynt August 28, 2024 02:15

MatthewHambley requested changes Aug 29, 2024

View reviewed changes

Made mpi and openmp default to False in the BuildConfig constructor.

2617322

Removed white space.

f165e46

hiker requested a review from MatthewHambley October 21, 2024 03:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mpi omp support #336

Mpi omp support #336

hiker commented Aug 21, 2024 •

edited

Loading

hiker commented Aug 28, 2024

MatthewHambley left a comment

MatthewHambley Aug 29, 2024

hiker Oct 21, 2024

MatthewHambley Aug 29, 2024

hiker Oct 21, 2024

MatthewHambley Aug 29, 2024

hiker Oct 21, 2024

MatthewHambley Aug 29, 2024

hiker Oct 21, 2024

MatthewHambley Aug 29, 2024

hiker Sep 2, 2024 •

edited

Loading

hiker Oct 2, 2024

MatthewHambley Aug 29, 2024

hiker Oct 21, 2024

MatthewHambley Aug 29, 2024

hiker Sep 2, 2024

MatthewHambley Aug 29, 2024

allynt Sep 3, 2024

hiker Oct 21, 2024

hiker commented Sep 2, 2024

MatthewHambley commented Sep 19, 2024

hiker commented Oct 21, 2024

		compiler, flags_config = handle_compiler_args(config, common_flags,
		path_flags)



		# ============================================================================
		class MpiGcc(Gcc):

		return BuildConfig('proj', ToolBox(), mpi=False, openmp=False,
		fab_workspace=tmp_path)

Mpi omp support #336

Are you sure you want to change the base?

Mpi omp support #336

Conversation

hiker commented Aug 21, 2024 • edited Loading

hiker commented Aug 28, 2024

MatthewHambley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiker Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiker commented Sep 2, 2024

MatthewHambley commented Sep 19, 2024

hiker commented Oct 21, 2024

hiker commented Aug 21, 2024 •

edited

Loading

hiker Sep 2, 2024 •

edited

Loading