Define threading env-vars only for threaded builds #1549

amametjanov · 2017-05-20T01:47:22Z

Adds SMP_PRESENT case-var, which is true if (BUILD_THREADED or NTHRDS_* >1)
Enable/disable env_mach_specific XML variables based on
attributes set in the case variables

[BFB]

jgfouca · 2017-05-21T22:07:30Z

@amametjanov , this is a clever implementation, but I doubt it would be accepted in its current implementation, but I think it's pretty close. I think the better way to do this is to add support for matching arbitrary case values to _match_attribs. I will try to be more specific in the comments.

jgfouca

Good idea, but needs refactor.

jgfouca · 2017-05-21T22:11:10Z

cime/config/acme/machines/config_machines.xml

-	       <arg name="xl_bg_spreadlayout"> --envs XL_BG_SPREADLAYOUT=YES</arg>
-               <arg name="omp_stacksize"> --envs OMP_STACKSIZE=64M</arg>
-               <arg name="thread_count"> --envs OMP_NUM_THREADS=$ENV{OMP_NUM_THREADS}</arg>
+               <arg SMP_PRESENT="TRUE" name="bg_threadlayout"> --envs BG_THREADLAYOUT=1</arg>


Instead, do this:

<arg name="extra_smp_stuff"> $ENV{ACME_EXTRA_SMP_STUFF}</arg>

The config file is already in acme/machines so does ACME have to be in the ENV variable?

@rljacob I was mostly just trying to ensure that the variable name didn't conflict with anything.

Matching at the arg tag is only limited to compiler, debug and mpilib variables. Why not extend the matching to other variables too.

@amametjanov , CIME doesn't do any matching for individual mpirun arguments:

# Now that we know the best match, compute the arguments if not exe_only: arg_node = self.get_optional_node("arguments", root=the_match) if arg_node is not None: arg_nodes = self.get_nodes("arg", root=arg_node) for arg_node in arg_nodes: arg_value = transform_vars(arg_node.text, case=case, subgroup=job, check_members=check_members, default=arg_node.get("default")) args.append(arg_value)

^ snippet from env_mach_specific.get_mpirun

mpirun args can be lengthy and this one-line change avoids repeating the whole mpirun-xml block for a different combination of args.

jgfouca · 2017-05-21T22:12:44Z

cime/config/acme/machines/config_machines.xml

@@ -1075,8 +1075,8 @@
    </module_system>
    <environment_variables>
      <env name="MPI_TYPE_MAX">10000</env>
-      <env name="OMP_DYNAMIC">FALSE</env>


Instead, do this:

<environment_variables> <env name="MPI_TYPE_MAX">10000</env> </environment_variables> <environment_variables SMP_PRESENT="TRUE"> <env name="OMP_DYNAMIC">FALSE</env> <env name="OMP_STACKSIZE">64M</env> <env name="ACME_EXTRA_SMP_STUFF">--envs XL_BG_SPREADLAYOUT=YES --envs OMP_STACKSIZE=64M --envs OMP_NUM_THREADS=$ENV{OMP_NUM_THREADS}</env> </environment_variables>

Testing for actual/resolved values of $ENV{OMP_NUM_THREADS} at case setup or build time shows that this variable's value isn't known until job actually runs. So $ENV{ACME_EXTRA_SMP_STUFF} will also be unresolved.

Ah, I see. Interesting problem. Maybe if you used $OMP_NUM_THREADS instead of $ENV{OMP_NUM_THREADS} it would leave it unresolved until the shell executed it.

Unlike vars like $COBALT_PARTNAME and $LID, $OMP_NUM_THREADS is known at case-setup-time.

jgfouca · 2017-05-21T22:14:42Z

cime/scripts/lib/CIME/XML/env_mach_specific.py

@@ -183,21 +183,26 @@ def _compute_actions(self, nodes, child_tag, compiler, debug, mpilib):

        return result

-    def _match_attribs(self, attribs, compiler, debug, mpilib):
-        if ("compiler" in attribs and
+    def _match_attribs(self, attribs, compiler=None, debug=None, mpilib=None):


Remove the =None changes.

Matching is hard-wired to only look at compiler, debug and mpilib vars: need to generalize. Also, making these parameters optional removes the need to supply actual values at call-site.

I agree that we need to generalize.

jgfouca · 2017-05-21T22:15:08Z

cime/scripts/lib/CIME/XML/env_mach_specific.py

            not self._match("TRUE" if debug else "FALSE", attribs["debug"].upper())):
            return False
        elif ("unit_testing" in attribs and
              not self._match("TRUE" if self._unit_testing else "FALSE",
                              attribs["unit_testing"].upper())):
            return False

+        # check for matches with env-vars
+        for attrib in attribs:


Keep this, except refactor to look at case object instead of environment.

It doesn't look like env_mach_specific.py initializes/uses case object, and didn't want to introduce unexpected side effects of loading a case object.

The call chain that uses this is:

case.load_env -> env_mach_specific.load_env -> env_mach_specific._get_modules_for_case -> env_mach_specific._compute_module_actions -> env_mach_specific._compute_actions -> env_mach_specific._match_attribs

So, you have a case object in that call chain that can be passed down. I don't see any other way to do this as we cannot expect every case setting to be in the environment.

jgfouca · 2017-05-21T22:15:48Z

cime/scripts/lib/CIME/XML/env_mach_specific.py

-                                               check_members=check_members,
-                                               default=arg_node.get("default"))
-                    args.append(arg_value)
+                    if (self._match_attribs(arg_node.attrib)):


Undo this change.

XML nodes under

<mpirun mpilib="default"> <executable>/usr/bin/runjob</executable> <arguments>

are matched/resolved/replaced in this function.

Side note: this section can also process elements with env-vars like

<arg name="thread_count"> --envs OMP_NUM_THREADS=$ENV{OMP_NUM_THREADS}</arg>

and a call to preview_run returns

MPIRUN: ERROR: Undefined env var 'OMP_NUM_THREADS'

That's why it is necessary to refer to env-vars at build-time on login nodes.

jgfouca · 2017-05-21T22:16:11Z

cime/scripts/lib/CIME/case.py

-        return bool(force_threaded) or self.thread_count > 1
+        smp_present = bool(force_threaded) or self.thread_count > 1
+        self.set_value("SMP_PRESENT", stringify_bool(smp_present))
+        os.environ["SMP_PRESENT"] = stringify_bool(smp_present)


Remove this line (add SMP_PRESENT to env)

amametjanov

Jim, env_mach_specific.py appears to be limited to the env-vars.

amametjanov · 2017-05-22T17:43:47Z

cime/scripts/lib/CIME/XML/env_mach_specific.py

            not self._match("TRUE" if debug else "FALSE", attribs["debug"].upper())):
            return False
        elif ("unit_testing" in attribs and
              not self._match("TRUE" if self._unit_testing else "FALSE",
                              attribs["unit_testing"].upper())):
            return False

+        # check for matches with env-vars
+        for attrib in attribs:


It doesn't look like env_mach_specific.py initializes/uses case object, and didn't want to introduce unexpected side effects of loading a case object.

amametjanov · 2017-05-22T17:48:43Z

cime/scripts/lib/CIME/XML/env_mach_specific.py

-                                               check_members=check_members,
-                                               default=arg_node.get("default"))
-                    args.append(arg_value)
+                    if (self._match_attribs(arg_node.attrib)):


XML nodes under

<mpirun mpilib="default"> <executable>/usr/bin/runjob</executable> <arguments>

are matched/resolved/replaced in this function.

Side note: this section can also process elements with env-vars like

<arg name="thread_count"> --envs OMP_NUM_THREADS=$ENV{OMP_NUM_THREADS}</arg>

and a call to preview_run returns

MPIRUN: ERROR: Undefined env var 'OMP_NUM_THREADS'

That's why it is necessary to refer to env-vars at build-time on login nodes.

amametjanov · 2017-05-22T17:50:31Z

cime/scripts/lib/CIME/XML/env_mach_specific.py

@@ -183,21 +183,26 @@ def _compute_actions(self, nodes, child_tag, compiler, debug, mpilib):

        return result

-    def _match_attribs(self, attribs, compiler, debug, mpilib):
-        if ("compiler" in attribs and
+    def _match_attribs(self, attribs, compiler=None, debug=None, mpilib=None):


Matching is hard-wired to only look at compiler, debug and mpilib vars: need to generalize. Also, making these parameters optional removes the need to supply actual values at call-site.

amametjanov · 2017-05-22T17:55:37Z

cime/config/acme/machines/config_machines.xml

-	       <arg name="xl_bg_spreadlayout"> --envs XL_BG_SPREADLAYOUT=YES</arg>
-               <arg name="omp_stacksize"> --envs OMP_STACKSIZE=64M</arg>
-               <arg name="thread_count"> --envs OMP_NUM_THREADS=$ENV{OMP_NUM_THREADS}</arg>
+               <arg SMP_PRESENT="TRUE" name="bg_threadlayout"> --envs BG_THREADLAYOUT=1</arg>


Matching at the arg tag is only limited to compiler, debug and mpilib variables. Why not extend the matching to other variables too.

amametjanov · 2017-05-22T17:58:37Z

cime/config/acme/machines/config_machines.xml

@@ -1075,8 +1075,8 @@
    </module_system>
    <environment_variables>
      <env name="MPI_TYPE_MAX">10000</env>
-      <env name="OMP_DYNAMIC">FALSE</env>


Testing for actual/resolved values of $ENV{OMP_NUM_THREADS} at case setup or build time shows that this variable's value isn't known until job actually runs. So $ENV{ACME_EXTRA_SMP_STUFF} will also be unresolved.

amametjanov

@jgfouca the update generalizes to case-vars now.

amametjanov · 2017-05-22T21:07:21Z

cime/config/acme/machines/config_machines.xml

-	       <arg name="xl_bg_spreadlayout"> --envs XL_BG_SPREADLAYOUT=YES</arg>
-               <arg name="omp_stacksize"> --envs OMP_STACKSIZE=64M</arg>
-               <arg name="thread_count"> --envs OMP_NUM_THREADS=$ENV{OMP_NUM_THREADS}</arg>
+               <arg SMP_PRESENT="TRUE" name="bg_threadlayout"> --envs BG_THREADLAYOUT=1</arg>


mpirun args can be lengthy and this one-line change avoids repeating the whole mpirun-xml block for a different combination of args.

amametjanov · 2017-05-22T21:09:32Z

cime/config/acme/machines/config_machines.xml

@@ -1075,8 +1075,8 @@
    </module_system>
    <environment_variables>
      <env name="MPI_TYPE_MAX">10000</env>
-      <env name="OMP_DYNAMIC">FALSE</env>


Unlike vars like $COBALT_PARTNAME and $LID, $OMP_NUM_THREADS is known at case-setup-time.

jgfouca · 2017-05-23T19:36:17Z

@amametjanov I made some changes to this branch that I think will make it more more likely for the ESMCI folks to accept. I also briefly tested it:

% ./create_test SMS.f19_g16_rx1.A --no-build
% cd $CASEDIR
% ./preview_run 
BATCH SUBMIT:
    case.test -> qsub   --cwd /projects/HiRes_EarthSys_2/acmetest/SMS.f19_g16_rx1.A.cetus_ibm.20170523_193017_g3q61f -A HiRes_EarthSys_2 -t 01:00:00 -n 16 -q default --mode script case.test 

MPIRUN: /usr/bin/runjob  --label short  --ranks-per-node 4  --np 64  --block $COBALT_PARTNAME $LOCARGS  --envs BG_THREADLAYOUT=1 --envs XL_BG_SPREADLAYOUT=YES --envs OMP_STACKSIZE=64M --envs OMP_NUM_THREADS=16 :  /projects/HiRes_EarthSys_2/acmetest/SMS.f19_g16_rx1.A.cetus_ibm.20170523_193017_g3q61f/bld/acme.exe  >> acme.log.$LID 2>&1

amametjanov · 2017-05-23T21:48:08Z

@jgfouca thanks for the updates. I noticed an additional export EXTRA_SMP_STUFF=None prior to an actual export in .env_mach_specific.sh, but we'll have to know that it's needed so that the variable is defined in both contexts. Did a little bit of a cleanup. Please proceed. Thank you.

jgfouca · 2017-05-23T21:50:55Z

@amametjanov will do, thanks.

Define threading env-vars only for threaded builds * Adds SMP_PRESENT case-var, which is true if (BUILD_THREADED or NTHRDS_* >1) * Enable/disable env_mach_specific XML variables based on attributes set in the case variables [BFB] * azamat/mira/add-smp-present: Cleanup BGQ SMP vars Fixes for cetus jgfouca mods to this branch Match env_mach_specific.xml tag attributes with any case-var value Define threading env-vars only for threaded builds

jgfouca · 2017-05-23T21:53:35Z

Merged to next.

This reverts commit f08d778, reversing changes made to 9233c4b. Bisect pointed to this as causing fails on melvin-next.

rljacob · 2017-05-26T04:32:13Z

reverted off of next. Bisect pointed to this one as causing recent build fails.

…(PR #1549)"" This reverts commit 5f14228.

Merge #2 for this PR. * azamat/mira/add-smp-present: Fix race condition

This reverts commit aabde09, reversing changes made to 5a48e63. Revert the second merge of this branch because still causing build fails on next.

rljacob · 2017-05-27T15:21:30Z

The attempt to fix the build problems didn't solve all of them so reverted from next again.

…to next (PR #1549)""" This reverts commit 5a48e63. Finish removing PR #1549

rljacob · 2017-05-28T15:36:12Z

Had to "revert revert revert" to fully remove this from next.

Merge #3 for this PR * azamat/mira/add-smp-present: Refactor a non-pythonic list join Remove very dangerous side effect from get_build_threaded Fix ERP Fix race condition Cleanup BGQ SMP vars Fixes for cetus jgfouca mods to this branch Match env_mach_specific.xml tag attributes with any case-var value Define threading env-vars only for threaded builds

It is NOT safe to do a set_value in just any old place in the code. [BFB]

Merge #4 for this PR. * azamat/mira/add-smp-present: More fixes

Updated mpi-serial from MCSclimate/mpi-serial Merged recent changes from MCSclimate/mpi-serial into CIME. Includes new hvector functions. Test suite: scripts_regression_test.py SMS_D_Mmpi-serial_Ld1.T42_T42.FSCM5A97.hobart_nag Test baseline: NA Test namelist changes: NA Test status: bit for bit Fixes #1549 User interface changes?: NA Code review:jedwards

Define threading env-vars only for threaded builds (PR #1549) * Adds SMP_PRESENT case-var, which is true if (BUILD_THREADED or NTHRDS_* >1) * Enable/disable env_mach_specific XML variables based on attributes set in the case variables [BFB]

amametjanov added the Mira label May 20, 2017

amametjanov requested a review from jgfouca May 20, 2017 01:47

rljacob assigned jgfouca May 20, 2017

rljacob added the CIME label May 20, 2017

jgfouca requested changes May 21, 2017

View reviewed changes

amametjanov commented May 22, 2017

View reviewed changes

jgfouca approved these changes May 23, 2017

View reviewed changes

rljacob added a commit that referenced this pull request May 26, 2017

Revert "Merge branch 'azamat/mira/add-smp-present' into next (PR #1549)"

5f14228

This reverts commit f08d778, reversing changes made to 9233c4b. Bisect pointed to this as causing fails on melvin-next.

jgfouca added a commit that referenced this pull request May 26, 2017

Revert "Revert "Merge branch 'azamat/mira/add-smp-present' into next …

5a48e63

…(PR #1549)"" This reverts commit 5f14228.

jgfouca added a commit that referenced this pull request May 26, 2017

Merge branch 'azamat/mira/add-smp-present' into next (PR #1549)

aabde09

Merge #2 for this PR. * azamat/mira/add-smp-present: Fix race condition

rljacob added a commit that referenced this pull request May 28, 2017

Revert "Revert "Revert "Merge branch 'azamat/mira/add-smp-present' in…

b7ff6bb

…to next (PR #1549)""" This reverts commit 5a48e63. Finish removing PR #1549

amametjanov and others added 6 commits May 30, 2017 13:08

Define threading env-vars only for threaded builds

4a7214d

Match env_mach_specific.xml tag attributes with any case-var value

1ffbb69

jgfouca mods to this branch

f5c3bd9

Fixes for cetus

9893647

Cleanup BGQ SMP vars

90dbf5d

Fix race condition

e3a6fba

jgfouca force-pushed the azamat/mira/add-smp-present branch from 1cd98fd to e3a6fba Compare May 30, 2017 19:09

Fix ERP

8d2acff

jgfouca added 2 commits May 30, 2017 17:37

Remove very dangerous side effect from get_build_threaded

ec11338

Refactor a non-pythonic list join

d9d7c66

More fixes

c77c59a

It is NOT safe to do a set_value in just any old place in the code. [BFB]

jgfouca added a commit that referenced this pull request May 31, 2017

Merge branch 'azamat/mira/add-smp-present' into next (PR #1549)

4ceaac8

Merge #4 for this PR. * azamat/mira/add-smp-present: More fixes

jgfouca merged commit b3fbd18 into master Jun 8, 2017

jgfouca deleted the azamat/mira/add-smp-present branch June 8, 2017 20:42

amametjanov mentioned this pull request Nov 15, 2017

Enable performance data archiving on Theta #1893

Merged

Define threading env-vars only for threaded builds #1549

Define threading env-vars only for threaded builds #1549

Conversation

amametjanov commented May 20, 2017 • edited Loading

jgfouca commented May 21, 2017

jgfouca 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

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

amametjanov 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

amametjanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgfouca commented May 23, 2017

amametjanov commented May 23, 2017

jgfouca commented May 23, 2017

jgfouca commented May 23, 2017

rljacob commented May 26, 2017

rljacob commented May 27, 2017

rljacob commented May 28, 2017

amametjanov commented May 20, 2017 •

edited

Loading