Overhaul parallel runs #188

yardasol · 2023-01-19T18:05:26Z

Summary of changes

This PR adds support for flexible distributed memory runs on various machines with the addition of the mpi_args parameter. Specifically, this PR:

Deprecates the -n command line argument in favor of the mpi_args parameter.
Changes the -d command line argument to the more standard -s/--threads.
Updates docstrings adds machinery to handle the new parameter in Depcode.run_depletion_step(), SerpentDepcode.run_depletion_step(), and OpenMCDepcode.run_depletion_step() functions.
Adds some machinery to app.py to handle the new variables.
Update the docpages
Add null type to certain object properties in the input schema.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Required for Merging

Associated Issues and PRs

Issue: closes Feature: Enable use of mpiexec flags when executing depletion codes via SaltProc #173

Associated Developers

Dev: @

Checklist for Reviewers

Reviewers should use this link to get to the
Review Checklist before they begin their review.

- remove the `core_number` and `node_number` arguments from Simulation; consistency changes - Fix the input schema by adding multiple types - Make the try-except block in read_main_input more robust

LukeSeifert

Looks good, I had a few comments on some minor things.

LukeSeifert · 2023-01-23T16:31:07Z

doc/fileformatspec/depcode_input.rst

+  :type:
+    ``string``, ``null``
+


It doesn't look like these changes tie in with the changes to overhauling parallel runs. Consider adding a note to the PR about these changes.

LukeSeifert · 2023-01-23T16:37:49Z

saltproc/app.py

+    d : int
+        Number of threads to use for shared-memory parallelism.


Replace d with s as done previously

LukeSeifert · 2023-01-23T16:38:53Z

saltproc/app.py

-                        depletion code simulation')
+                        default=None,
+                        help='Number of threads to use for shared-memory \
+                        parallelism.')
    parser.add_argument('-i',      # main input file
                        type=str,
                        default=None,
                        help='path and name of SaltProc main input file')


Match capitalization in the help strings

LukeSeifert · 2023-01-23T16:41:09Z

saltproc/input_schema.json

@@ -270,12 +281,12 @@
                    "type": "boolean",
                    "default": false}
            },
-            "default": {},
-            "required": ["sim_name"]
+            "required": ["sim_name", "db_name"]


I thought db_name was previously made optional since there was a default setting?

LukeSeifert · 2023-01-23T16:42:20Z

saltproc/input_schema.json

            "required": ["volume", "mass_flowrate", "power_levels", "depletion_timesteps", "timestep_units"]
        }
    },
-    "required": ["proc_input_file", "dot_input_file", "output_path", "depcode", "simulation", "reactor"]
+    "required": ["proc_input_file", "dot_input_file", "output_path", "mpi_args", "depcode", "simulation", "reactor"]


How come mpi_args is required, isn't it given a default value of None?

LukeSeifert · 2023-01-23T16:43:11Z

saltproc/openmc_depcode.py

+                '--directory',
+                str(self.output_path)]
+        if mpi_args is not None:
+            args = mpi_args + args

        print('Running %s' % (self.codename))
        # TODO: Need to figure out how to adapt this to openmc


Is this TODO line still needed?

LukeSeifert · 2023-01-23T18:22:55Z

saltproc/openmc_depcode.py

-    def run_depletion_step(self, cores, nodes):
-        """Runs a depletion step in OpenMC as a subprocess with the given
-        parameters.
+    def run_depletion_step(self, mpi_args=None, threads=None):


I don't see threads called in this function, is it needed here?

It's needed in the signature, but it isn't used in the function.

LukeSeifert · 2023-01-23T18:24:53Z

saltproc/serpent_depcode.py

+        if threads is not None:
+            args = args + ['-omp', str(threads)]


How come this isn't in OpenMC run_depletion_step?

There isn't currently a way to specify how many OpenMC threads to use when using depletion outside of an environment variable. Otherwise the transport simulation will use as many threads as there are on the machine/node.

LukeSeifert · 2023-01-23T18:26:57Z

tests/conftest.py

@@ -27,13 +27,13 @@ def serpent_runtime(cwd, tmpdir_factory):
    """SaltProc objects for Serpent unit tests"""
    saltproc_input = str(cwd / 'serpent_data' / 'tap_input.json')
    depcode_input, simulation_input, reactor_input = \
-        read_main_input(saltproc_input)[3]
+        read_main_input(saltproc_input)[4]


Can you rewrite this in a way that doesn't use a hard-coded value?

I can't think of any, other than using -1 instead of 4.

LukeSeifert · 2023-01-23T18:27:49Z

tests/conftest.py

@@ -65,7 +65,7 @@ def openmc_runtime(cwd, tmpdir_factory):
    """SaltProc objects for OpenMC unit tests"""
    saltproc_input = str(cwd / 'openmc_data' / 'tap_input.json')
    depcode_input, simulation_input, reactor_input = \
-        read_main_input(saltproc_input)[3]
+        read_main_input(saltproc_input)[4]


yardasol · 2023-01-25T17:12:04Z

@samgdotson @abachma2 bump

LukeSeifert

Looks good to me, let me know if you want me to merge.

samgdotson

I don't have anything to add here, these are pretty straightforward changes. However, the abc.py file is still called abc.py which should be changd.

yardasol · 2023-01-26T22:31:21Z

I don't have anything to add here, these are pretty straightforward changes. However, the abc.py file is still called abc.py which should be changd.

As I've stated before, this will be happening in a future PR, and there is already an issue in the repo for this.

yardasol · 2023-01-27T18:45:46Z

@LukeSeifert is this good to merge?

…ments Overhaul parallel runs 419447d

…arguments Overhaul parallel runs 419447d

yardasol added 2 commits January 19, 2023 11:59

add mpi_args parameter, remove -n CL argument

f4429d5

update docpages and README

e5cbc3d

yardasol added Status:4-In Progress Priority:1-Critical Comp:Input labels Jan 19, 2023

yardasol changed the title ~~Mpi arguments~~ Overhaul parallel runs Jan 19, 2023

yardasol added 3 commits January 19, 2023 14:57

syntax fix

2830233

Merge remote-tracking branch 'upstream/master' into mpi-arguments

af7625c

consistency changes in test suite related to mpi_args

4e77f14

- remove the `core_number` and `node_number` arguments from Simulation; consistency changes - Fix the input schema by adding multiple types - Make the try-except block in read_main_input more robust

yardasol mentioned this pull request Jan 20, 2023

[Bug]: Fix JSON Schema validation and add test coverage for all inputs #184

Open

update docpages and releasenotes

2bd7cc9

yardasol marked this pull request as ready for review January 20, 2023 20:55

yardasol requested review from samgdotson, abachma2 and LukeSeifert January 20, 2023 20:56

LukeSeifert reviewed Jan 23, 2023

View reviewed changes

changes from @LukeSeifert's review

191a0b5

yardasol requested a review from LukeSeifert January 25, 2023 17:12

LukeSeifert approved these changes Jan 25, 2023

View reviewed changes

samgdotson reviewed Jan 26, 2023

View reviewed changes

LukeSeifert merged commit 419447d into arfc:master Jan 27, 2023

github-actions bot pushed a commit that referenced this pull request Jan 27, 2023

update docs at commit: Merge pull request #188 from yardasol/mpi-argu…

2a2c7ae

…ments Overhaul parallel runs 419447d

github-actions bot pushed a commit to khurrumsaleem/saltproc that referenced this pull request Jan 30, 2023

update docs at commit: Merge pull request arfc#188 from yardasol/mpi-…

f154e5e

…arguments Overhaul parallel runs 419447d

yardasol removed the Status:4-In Progress label Mar 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul parallel runs #188

Overhaul parallel runs #188

yardasol commented Jan 19, 2023 •

edited

Loading

LukeSeifert left a comment

LukeSeifert Jan 23, 2023

yardasol Jan 24, 2023

LukeSeifert Jan 23, 2023

yardasol Jan 24, 2023

LukeSeifert Jan 23, 2023

LukeSeifert Jan 23, 2023

LukeSeifert Jan 23, 2023

LukeSeifert Jan 23, 2023

LukeSeifert Jan 23, 2023

yardasol Jan 24, 2023

LukeSeifert Jan 23, 2023

yardasol Jan 24, 2023

LukeSeifert Jan 23, 2023

yardasol Jan 24, 2023

LukeSeifert Jan 23, 2023

yardasol commented Jan 25, 2023

LukeSeifert left a comment

samgdotson left a comment

yardasol commented Jan 26, 2023

yardasol commented Jan 27, 2023

		d : int
		Number of threads to use for shared-memory parallelism.

Overhaul parallel runs #188

Overhaul parallel runs #188

Conversation

yardasol commented Jan 19, 2023 • edited Loading

Summary of changes

Types of changes

Required for Merging

Associated Issues and PRs

Associated Developers

Checklist for Reviewers

LukeSeifert 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

yardasol commented Jan 25, 2023

LukeSeifert left a comment

Choose a reason for hiding this comment

samgdotson left a comment

Choose a reason for hiding this comment

yardasol commented Jan 26, 2023

yardasol commented Jan 27, 2023

yardasol commented Jan 19, 2023 •

edited

Loading