Launch Arg Builders Format Input for Launchers #620

MattToast · 2024-06-24T16:46:21Z

Add a LaunchArgBuilder.finalize method to the builder interface. The method takes something that can be formatted into an executable and an environment mapping and returns something that can be used as input for a launcher to start. VERY simple implementations have been added for all settings created so far.

Create a bare minimum ShellLauncher to launch settings formatted as Sequence[str] that can be sent to a Popen process. This can be used as a template to create a LocalLauncher, SlurmLauncher, etc.

Create a Dispatcher class and a global DEFAULT_DISPATCHER instance. This can be used by the experiment class to get which type of launcher are to be used for a specific LaunchSettingsBuilder by maintaining an internal mapping of settings type -> launcher type. This mapping can be built in a type safe way by calling Dispatcher.disptach. Experiments will likely be able to change how they launch each settings type by taking in a Dispatcher as a dependency, with a default being the DEFAULT_DISPATCHER. e.g.

class Experiment:
    ...
    def start(
        self,
        job: Job,
        *jobs: Job,
        dispatcher: Dispatcher = DEFAULT_DISPATCHER
    ) -> LaunchedJobIDs: ...
    ...

codecov · 2024-06-26T22:56:01Z

Codecov Report

Attention: Patch coverage is 62.08333% with 91 lines in your changes missing coverage. Please review.

Please upload report for BASE (smartsim-refactor@9b511f1). Learn more about missing BASE report.

Additional details and impacted files

@@                 Coverage Diff                  @@
##             smartsim-refactor     #620   +/-   ##
====================================================
  Coverage                     ?   34.24%           
====================================================
  Files                        ?      108           
  Lines                        ?     6460           
  Branches                     ?        0           
====================================================
  Hits                         ?     2212           
  Misses                       ?     4248           
  Partials                     ?        0

Files	Coverage Δ
smartsim/_core/config/config.py	`62.85% <100.00%> (ø)`
smartsim/error/errors.py	`59.18% <100.00%> (ø)`
smartsim/settings/arguments/__init__.py	`100.00% <100.00%> (ø)`
smartsim/settings/arguments/batch/__init__.py	`100.00% <100.00%> (ø)`
smartsim/settings/arguments/batchArguments.py	`76.92% <100.00%> (ø)`
smartsim/settings/arguments/launch/__init__.py	`100.00% <100.00%> (ø)`
smartsim/settings/arguments/launch/local.py	`48.38% <100.00%> (ø)`
smartsim/types.py	`100.00% <100.00%> (ø)`
smartsim/_core/utils/helpers.py	`33.00% <75.00%> (ø)`
smartsim/settings/arguments/launch/dragon.py	`61.11% <75.00%> (ø)`
... and 8 more

REBASEME: moar mpi REBASEME: fmt mpi

mellis13

Just submitting some comments and questions. We discussed offline about the refactor the arg builder, and that feedback was not included in my comments.

smartsim/_core/launcher/dragon/dragonConnector.py

smartsim/experiment.py

smartsim/settings/launchSettings.py

smartsim/settings/builders/launchArgBuilder.py

smartsim/settings/dispatch.py

amandarichardsonn

Questions on Questions on Questions, but awesome work! I am learning a lot

smartsim/experiment.py

amandarichardsonn · 2024-07-23T16:41:34Z

smartsim/settings/dispatch.py

+    from smartsim.experiment import Experiment
+    from smartsim.settings.arguments import LaunchArguments
+
+_Ts = TypeVarTuple("_Ts")


All these TypeVars, are they just existing 'for now'?

Nope! We need them to keep our classes/methods/functions generic.

Old school python typing requires that generic types be declared at (iirc) the top level of a python module. You can think of this python function

class SupportsAdd(Protocol): def __add__(self, other: Self) -> Self: ... T = TypeVar("T", bound=SupportsAdd) def my_add(a: T, b: T) -> T: return a + b

as roughly equivalent to this in C++

#template<typename T> T my_add(T a, T b) { return a + b }

This is syntax is quite unintuitive, imo, but luckily was changed in 3.12 to this!

def my_add[T: SupportsAdd](a: T, b: T) -> T: return a + b

which, again imo, is much more readable and simple to understand! We just cannot use it yet for backward compatibility with python <3.12 😢

Also, apparently mypy doesn't fully support the syntax yet, lol

smartsim/settings/dispatch.py

amandarichardsonn · 2024-07-23T17:00:26Z

smartsim/settings/dispatch.py

+            return None
+        return register
+
+    def get_dispatch(


Why accept two different types for args when it will always be of type LaunchArguments?

mostly just convenience! that way if I have

args: LaunchArguments = ...

I can call

dispatcher.get_dispatch(args)

instead of

dispatcher.get_dispatch(type(args))

If you think this is more confusing than it is helpful, I'd be willing to remove it!

amandarichardsonn · 2024-07-23T17:08:35Z

smartsim/experiment.py

+            # ---------------------------------------------------------------------
+            exe = t.cast("ExecutableProtocol", job.entity)
+            # <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
+            dispatch = dispatcher.get_dispatch(args)


Okay so I am trying to understand this, start_jobs accepts a Dispatcher and defaults to a Dispatcher you created in dispatch.py where the Dispatch init accepts the type of LaunchArgument child class. But you are not passing in the LaunchArgument type when initializing the Dispatcher so how can you run .get_dispatch when the self._dispatch_registry seems like it would be empty?

But you are not passing in the LaunchArgument type when initializing the Dispatcher so how can you run .get_dispatch when the self._dispatch_registry seems like it would be empty?

Absolutely correct BUT the dispatch function in smartsim.settings.dispatch is actually a pointer to the DEFAULT_DISPATHCER.dispatch method. That way when a user creates a new settings and python loads the definition of the LaunchArguments subclass it will automatically be added in the DEFAULT_DISPATCHER._dispatch_registry

from smartsim.settings.dispatch import dispatch from smartsim.settings.arguments.launchArguments import LaunchArguments @dispatch(...) # <-- Added into the registry here # before the `__init__` method is ever even called class MyLaunchArguments(LaunchArguments): ...

smartsim/experiment.py

amandarichardsonn · 2024-07-23T17:19:38Z

smartsim/experiment.py

+            # it easier to monitor job statuses
+            # pylint: disable-next=protected-access
+            self._active_launchers.add(launch_config._adapted_launcher)
+            return launch_config.start(exe, env)


My brain might explode, but that is okay. So here, you're calling .start but where is make_shell_format_fn actually being ran? I see it set in each launch file? I know its being run?

make_shell_format_fn is being called when smartsim.settings.arguments.launch.alps, smartsim.settings.launch.mpi, etc. are loaded by the interpreter and the function that is returned from that call is added into the default dispatcher's dispatch registry.

The dispatch registration then passes it (the make_shell_format_fn returned function) into a launcher adapter where it is finally called when the LauncherAdapter.start method is finally called (at that point it is at the self._adapt instance var)!

smartsim/settings/arguments/launchArguments.py

smartsim/settings/dispatch.py

smartsim/experiment.py

smartsim/settings/launchSettings.py

smartsim/settings/dispatch.py

tests/test_experiment.py

…rs-fmt

amandarichardsonn

Hark, fellow coder! Behold this wondrous creation, forged in the fires of logic and threaded with the silken strands of syntax. With a flourish of my digital wand, I hereby grant my blessing upon this noble pull request!

amandarichardsonn · 2024-07-26T15:49:34Z

smartsim/experiment.py

-        exp_path: t.Optional[str] = None,
-        launcher: str = "local",
-    ):
+    def __init__(self, name: str, exp_path: str | None = None):


Is there a significant diff between types exp_path: t.Optional[str] = None and exp_path: str | None = None? Or is this just personal preference

They are 100% equivalent! I just like the latter because:

It's slightly less to type, lol

I think T | None does a better job of communicating that this parameter is nullable than Optional[T]

Oh! One last caveat!!

The latter syntax is only available with Python >= 3.10 or with the from __future__ import annotations flag for the interpreter. But I'm also on a bit of a mission to start moving more of our modules over to using from __future__ import annotations anyways!

smartsim/settings/arguments/batch/lsf.py

smartsim/settings/arguments/batch/pbs.py

smartsim/settings/arguments/launch/alps.py

smartsim/settings/arguments/launch/dragon.py

amandarichardsonn · 2024-07-26T15:59:06Z

smartsim/settings/arguments/launchArguments.py

@@ -71,6 +85,15 @@ def format_comma_sep_env_vars(
        Slurm takes exports in comma separated lists
        the list starts with all as to not disturb the rest of the environment
        for more information on this, see the slurm documentation for srun
+
+        .. warning::


Is there a chance that we will forget about this comment?

Don't worry! I will make a ticket for this/other methods!

amandarichardsonn · 2024-07-26T16:01:29Z

smartsim/settings/dispatch.py

+    from smartsim.settings.arguments import LaunchArguments
+
+_Ts = TypeVarTuple("_Ts")
+_T_contra = t.TypeVar("_T_contra", contravariant=True)


did you by chance miss this one? _T_contra?

Uhh, I can try, but it truly is "a type variable that can bind to anything and is contravariant"... There's not much more context that I can add :(

smartsim/settings/dispatch.py

MattToast added type: design Issues related to architecture and code design area: settings Issues related to Batch or Run settings area: launcher Issues related to any of the launchers within SmartSim area: api Issues related to API changes ignore-for-release labels Jun 24, 2024

MattToast self-assigned this Jun 24, 2024

MattToast force-pushed the builders-fmt branch 2 times, most recently from 396306f to cabc2a0 Compare June 26, 2024 22:39

MattToast force-pushed the smartsim-refactor branch 2 times, most recently from 8a3889e to d15e951 Compare June 26, 2024 22:46

MattToast force-pushed the builders-fmt branch from cabc2a0 to da2b2c4 Compare June 26, 2024 22:48

MattToast force-pushed the smartsim-refactor branch from d15e951 to e9d8eca Compare June 26, 2024 22:57

MattToast added 11 commits June 26, 2024 17:57

Builders are generic, have a format method

42f7ff1

Impl slurm

f76ee4f

Impl local

3027721

Impl jsrun

6266b75

Impl mpi{run,exec}, orterun

bbb5152

REBASEME: moar mpi REBASEME: fmt mpi

Impl aprun

b96a160

Impl dragon

dc1cf0b

Type errors supressed for now

e555342

Add a dispatcher class to send built settings to a launcher

9161125

Isort/Black

d765718

Env: dict -> Mapping

01393c7

MattToast force-pushed the builders-fmt branch from da2b2c4 to 01393c7 Compare June 26, 2024 22:58

MattToast added 3 commits July 2, 2024 11:47

Organize dispatch file, call out work to do

e504ccf

Wire up dispatch dragon builder to dragon launcher

910b2d9

Import sort

f4ebada

MattToast requested review from juliaputko and amandarichardsonn July 2, 2024 17:24

mellis13 reviewed Jul 17, 2024

View reviewed changes

MattToast added 3 commits July 19, 2024 20:45

Dispatcher takes a format function with launcher like

4e9f5b2

Re-wrire up default dispatcher

0b9ec1a

Add tests for new dispatch API, make old tests pass

6d5a3c4

MattToast force-pushed the builders-fmt branch from f9684d5 to 6d5a3c4 Compare July 20, 2024 01:53

MattToast added 5 commits July 19, 2024 22:37

Upper pin typing extensions (thanks TF)

6b7943a

Make 3.9 compatable, lint, better names

82ee19f

Address reviewer comments

36bcfbb

Add tests for Experiment.start_jobs

0a1ebba

Rename Builder -> Arguments

b729e91

MattToast requested review from mellis13 and amandarichardsonn July 22, 2024 18:39

amandarichardsonn reviewed Jul 23, 2024

View reviewed changes

mellis13 reviewed Jul 23, 2024

View reviewed changes

MattToast added 2 commits July 23, 2024 21:56

Remove Experiment._control

f0bf80f

Address some reviewer feedback

7a0b893

amandarichardsonn reviewed Jul 24, 2024

View reviewed changes

smartsim/settings/dispatch.py Show resolved Hide resolved

amandarichardsonn reviewed Jul 24, 2024

View reviewed changes

smartsim/settings/dispatch.py Show resolved Hide resolved

amandarichardsonn reviewed Jul 24, 2024

View reviewed changes

tests/test_experiment.py Show resolved Hide resolved

MattToast added 3 commits July 24, 2024 14:38

Moar reviewer feedback

9984d18

Moar reviewer requested docs

dbe581d

Merge remote-tracking branch 'upstream/smartsim-refactor' into builde…

7327375

…rs-fmt

MattToast requested review from mellis13 and amandarichardsonn July 24, 2024 22:43

MattToast added 2 commits July 24, 2024 17:44

typo

0b4e65d

Merge remote-tracking branch 'upstream/smartsim-refactor' into builde…

50f15c0

…rs-fmt

amandarichardsonn approved these changes Jul 26, 2024

View reviewed changes

Address (online and offline) reviewer comments

7bcc8fd

MattToast merged commit bd21957 into CrayLabs:smartsim-refactor Jul 27, 2024
32 of 33 checks passed

MattToast deleted the builders-fmt branch August 9, 2024 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Launch Arg Builders Format Input for Launchers #620

Launch Arg Builders Format Input for Launchers #620

MattToast commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 26, 2024 •

edited

Loading

mellis13 left a comment

amandarichardsonn left a comment

amandarichardsonn Jul 23, 2024

MattToast Jul 23, 2024 •

edited

Loading

MattToast Jul 23, 2024

amandarichardsonn Jul 23, 2024

MattToast Jul 23, 2024

amandarichardsonn Jul 23, 2024

MattToast Jul 24, 2024

amandarichardsonn Jul 23, 2024

MattToast Jul 24, 2024

amandarichardsonn left a comment

amandarichardsonn Jul 26, 2024

MattToast Jul 26, 2024

MattToast Jul 26, 2024

amandarichardsonn Jul 26, 2024

MattToast Jul 26, 2024

amandarichardsonn Jul 26, 2024

MattToast Jul 26, 2024

Launch Arg Builders Format Input for Launchers #620

Launch Arg Builders Format Input for Launchers #620

Conversation

MattToast commented Jun 24, 2024 • edited Loading

codecov bot commented Jun 26, 2024 • edited Loading

Codecov Report

mellis13 left a comment

Choose a reason for hiding this comment

amandarichardsonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattToast Jul 23, 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

amandarichardsonn 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

MattToast commented Jun 24, 2024 •

edited

Loading

codecov bot commented Jun 26, 2024 •

edited

Loading

MattToast Jul 23, 2024 •

edited

Loading