fixing horace mex and disentangling various mex options. #1754

abuts · 2024-09-27T16:38:54Z

Fixed Re #1753.

These are small changes necessary for using simple mex and MPI mex independently.

also fixed horace_mex to be able to help users without cmake

should be dealt with after Re #1752 is merged

…ex.m

…rm.m and parallel_config.m # Conflicts: # herbert_core/classes/MPIFramework/@ClusterSlurm/ClusterSlurm.m

cmarooney-stfc

one query about mpi labelling as herbert

cmarooney-stfc · 2024-10-02T08:03:43Z

horace_core/admin/check_horace_mex.m

 % function checks if horace mex files are compiled correctly and returns
 % their version string.
 %
 % Usage:
 %   >>[rez, n_errors] = check_horace_mex();
-%   >>[rez, n_errors, can_use_mex_4_combine] = check_horace_mex();
+%   >>[rez, n_errors, can_use_mex_4_combine,can_use_herbert_mpi] = check_horace_mex();


unclear why herbert is appearing here in the description

because parallel computing toolbox uses Matlab MPI and Herbert may use Poor man MPI which is not an MPI but pretends to be such (writes/reads/deletes files pretending to be MPI messages). And may use cpp_communicator which supports proper MPI implementation compiled for Herbert. The last option, is validated here.

cmarooney-stfc · 2024-10-02T08:04:34Z

horace_core/configuration/@hor_config/hor_config.m

@@ -296,14 +296,20 @@
            if use
                % Configure mex usage
                % --------------------
-                [~, n_errors, can_combine_with_mex] = check_horace_mex();
+                [~, n_errors, can_combine_with_mex,can_use_herbert_mpi] = check_horace_mex();


as above unclear why herbert is appearing here

may be gave this variable better name

oerc0122 · 2024-10-02T13:29:26Z

horace_core/admin/check_horace_mex.m

@@ -1,17 +1,29 @@
-function [rez, n_errors, can_use_mex_4_combine] = check_horace_mex()
+function [rez, n_errors, can_use_mex_4_combine,can_use_custom_MPI] = check_horace_mex()


What is custom here? What makes it custom? Do you mean system?

custom means created by us, not provided by Matlab.

oerc0122 · 2024-10-02T13:31:50Z

herbert_core/classes/MPIFramework/@parallel_config/parallel_config.m

@@ -491,7 +491,13 @@
        function obj = set.parallel_threads(obj,n_threads)
            n_threads = floor(n_threads);
            n_workers = get_or_restore_field(obj, 'parallel_workers_number');
-            n_poss_threads = floor(obj.n_cores/n_workers);
+            if ispc % let's assume pc is Intel so threades are double number of cores


What do you mean "assume it's intel"? Do you mean it has hyperthreading? You should be careful
a) because hyperthreading may be captured in n_cores or it may not. Sometimes you have hyperthreading mapped as a logical (but not a physical) processor other times it may just be a hidden feature handled internally as in some ARM systems.
b) Because hyperthreading may not always be advantageous to use if you're already saturating the processors with work and can result in slowdown.

oerc0122 · 2024-10-02T13:32:02Z

herbert_core/classes/MPIFramework/@parallel_config/parallel_config.m

+            if ispc % let's assume pc is Intel so threades are double number of cores
+                % this estimeat will not be too wrong even if it is AMD,


Suggested change

if ispc % let's assume pc is Intel so threades are double number of cores

% this estimeat will not be too wrong even if it is AMD,

if ispc % let's assume pc is Intel so threads are double number of cores

% this estimate will not be too wrong even if it is AMD,

oerc0122 · 2024-10-02T13:32:17Z

admin/horace_mex_mpi.m

@@ -5,7 +5,7 @@
 % Manually modify this script to specify the mpi libraries location in your
 % system.
 %
-use_her_mpich = false;
+use_her_mpich = true;


Why are we changing defaults here?

because I was trying to build cpp_communicator using the library, we provide with Herbert and it worked. Probably it is a bit easier to build cpp_communicator with the libraties we provide with Herbert, than install external MPI on a system and then build cpp_communicator (use_her_mpich=false -- look for MPI installed on a system)

There are actually little difference between these two options as compiling cpp_communicator is a simpler task than making it to work, which I will try to explain in a fresh ticket.

As from Slack earlier: two things. One, if it needs that much explanation, then it should be documented in the code. Please add a substantial comment saying what is going on there. Two, I don't understand your explanation. It sounds more as if there was a temporary glitch in the build process rather than something fundamental that requires this change. Please can you write a paragraph for comments and run them by us. If need be we can use the sprint review slot tomorrow to discuss it.

There are generally not advisable to describe in words the things the code is doing. It is usually clear enough from the code itself. You should describe the meanings if input/ouptut variables and describe intentions. All this is present here.

This requests violates this principle, but it is not difficult to add what the code is doing adding couple of sentences, so I am adding it in commit 4205739

oerc0122 · 2024-10-02T13:33:33Z

horace_core/configuration/@hor_config/hor_config.m

+                end
+                if ~can_use_custom_mpi
+                    pc = parallel_config;
+                    if ismember(pc.parallel_cluster,{'mpiexec_mpi','slurm_mpi'})


Why if these two are present in parallel_cluster would it default to herbert? What is this if statement actually checking?

because ~can_use_custom_mpi means that cpp_communicator does not work. These two frameworks use cpp_communicator as the transport, so they are not avaliable if it does not work.

cmarooney-stfc

Agreeing with Jacob that the issues he comments on need explaining. Not adding any changes required myself.

I think this needs a more comprehensive explanation going back to first principles; the changes and comments assume the reader knows something about these issues, which may not be the case. Please work out how to tell this story in the code.

…(but related to the alignment ticket) and made to push the build with changed integration scripts

…ariables

abuts requested review from oerc0122 and cmarooney-stfc September 27, 2024 16:38

abuts changed the title ~~fixing horace mex and dientangling various mex options.~~ fixing horace mex and disentangling various mex options. Sep 27, 2024

abuts added 5 commits September 30, 2024 17:33

Re #1753 disable error for failing cpp_communicator in check_horace_m…

d134296

…ex.m

Re #1753 disentangle parallel options and use_mex option

8cf800c

Re #1753 fixed horace_mex using Matlab

58d12d1

Re #1753 validated horace_mex_mpi

44d8d22

Re #1753 should fix test_mpi_wrapper and better warning in ClusterSlu…

95ae066

…rm.m and parallel_config.m # Conflicts: # herbert_core/classes/MPIFramework/@ClusterSlurm/ClusterSlurm.m

abuts force-pushed the 1753_horace_mex branch from 1ee8f78 to 95ae066 Compare September 30, 2024 20:45

cmarooney-stfc requested changes Oct 2, 2024

View reviewed changes

Merge branch 'master' into 1753_horace_mex

d0fc160

abuts requested a review from cmarooney-stfc October 2, 2024 12:52

Re #1753 slightly better description of one new property

91a737d

oerc0122 requested changes Oct 2, 2024

View reviewed changes

cmarooney-stfc reviewed Oct 3, 2024

View reviewed changes

Re #1753 -- minor comments (typos) identified by review.

3ccf790

abuts requested review from oerc0122 and cmarooney-stfc October 3, 2024 11:24

abuts added 3 commits October 9, 2024 19:20

Merge branch 'master' into 1753_horace_mex

f41ae85

Re #1753 remove redundant code unrelated to main topic of the ticket …

71f1a0b

…(but related to the alignment ticket) and made to push the build with changed integration scripts

Re #1753 changes from review. Couple of words about meaning of some v…

4205739

…ariables

abuts merged commit 8f71d35 into master Oct 21, 2024
5 checks passed

abuts deleted the 1753_horace_mex branch October 21, 2024 15:03

abuts mentioned this pull request Oct 21, 2024

horace_mex and issues with cpp_communicator #1753

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing horace mex and disentangling various mex options. #1754

fixing horace mex and disentangling various mex options. #1754

abuts commented Sep 27, 2024 •

edited

Loading

cmarooney-stfc left a comment

cmarooney-stfc Oct 2, 2024

abuts Oct 2, 2024 •

edited

Loading

cmarooney-stfc Oct 2, 2024

abuts Oct 2, 2024

oerc0122 Oct 2, 2024

abuts Oct 2, 2024

oerc0122 Oct 2, 2024 •

edited

Loading

oerc0122 Oct 2, 2024

oerc0122 Oct 2, 2024

abuts Oct 2, 2024 •

edited

Loading

cmarooney-stfc Oct 10, 2024

abuts Oct 10, 2024 •

edited

Loading

oerc0122 Oct 2, 2024

abuts Oct 2, 2024

cmarooney-stfc left a comment

		@@ -1,17 +1,29 @@
		function [rez, n_errors, can_use_mex_4_combine] = check_horace_mex()
		function [rez, n_errors, can_use_mex_4_combine,can_use_custom_MPI] = check_horace_mex()

		if ispc % let's assume pc is Intel so threades are double number of cores
		% this estimeat will not be too wrong even if it is AMD,

fixing horace mex and disentangling various mex options. #1754

fixing horace mex and disentangling various mex options. #1754

Conversation

abuts commented Sep 27, 2024 • edited Loading

cmarooney-stfc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abuts Oct 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

oerc0122 Oct 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

abuts Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abuts Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmarooney-stfc left a comment

Choose a reason for hiding this comment

abuts commented Sep 27, 2024 •

edited

Loading

abuts Oct 2, 2024 •

edited

Loading

oerc0122 Oct 2, 2024 •

edited

Loading

abuts Oct 2, 2024 •

edited

Loading

abuts Oct 10, 2024 •

edited

Loading