Compound Entity and Ensemble #605
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## smartsim-refactor #605 +/- ##
====================================================
Coverage ? 33.26%
====================================================
Files ? 107
Lines ? 6401
Branches ? 0
====================================================
Hits ? 2129
Misses ? 4272
Partials ? 0
|
Ensemble creation strategy implemented. [ reviewed by @MattToast ] [ committed by @amandarichardsonn ]
amandarichardsonn
left a comment
amandarichardsonn
left a comment
There was a problem hiding this comment.
Lets go! Awesome work and thank you for fixing those type errors, some suggestions but feel free to say nah
smartsim/entity/_new_ensemble.py
Outdated
| copy.deepcopy(exe_arg_parameters) if exe_arg_parameters else {} | ||
| ) | ||
| self.files = copy.deepcopy(files) if files else EntityFiles() | ||
| self.file_parameters = dict(file_parameters) if file_parameters else {} |
There was a problem hiding this comment.
do you mind adding a deep_copy for the file_parameters, just to be consistent with exe_arg_parameters? That is a my bad MERP
There was a problem hiding this comment.
What are your thoughts on this, I am thinking that the attributes that do not need to be deepcopied are replicas, max_permutations and strategy, just bc they do not live on to Application and are not reused
There was a problem hiding this comment.
or hmm maybe deepcopy is just something to worry about for Application?
There was a problem hiding this comment.
I think the general thought process is that we should make copies of anything that is mutable when it is past to the ensemble constructor.
Because strings are immutable in python, a deep-copy of a dict[str, str] (or really any MutableMapping[str, str]) is functionally equivalent to making a shallow copy! I chose dict over copy.copy or copy.deepcopy simply so that any helper methods know the actual type of the container, and so that user know they could modify the collection after construction (should they choose)
original_params = {"SOME": "PARAM"}
ens = Ensemeble(..., file_parameters=original_params, ...)
# Wait I need to change those params for some reason!!
# But know that I can because the attr is typed as a dict!
ens.file_parameters["SOME"] = "OTHER_PARAMETER" # passes type check!
assert ens.file_parameters != original_params # passes!There was a problem hiding this comment.
To that end, I think we need:
- a deep copy of
filesas its a complex user (SmartSim) defined class - a deep copy of
exe_args_parametersas its a container of containers of immutable elements - shallow copies of
file_parametersandexe_argsas they are containers of immutable elements - No copies of any other parameters as they are all immutable
What do you think?
MattToast
force-pushed
the
smartsim-refactor
branch
2 times, most recently
from
d15e951 to
e9d8eca
Compare
| # self.exe = [exe] if run_settings.container else [expand_exe_path(exe)] | ||
| self.exe_args = exe_args or [] | ||
| self.params = params or {} | ||
| self.params = params.copy() if params else {} |
There was a problem hiding this comment.
why shallow copy of params here? and not deep? but deep for files?
There was a problem hiding this comment.
Strings are immutable in python, a shallow copying is container of immutable objects is functionally equivalent to making deep copy because in order to change any of elements in the container it must be replaced wholesale!
See here for my decision process for what I chose to deep/shallow/not copy, but I could be very easily convinced to make this a deep copy just for consistencies stake if you'd like!
| new_strat = lambda params, exe_args, nmax: [] | ||
| strategies._register("new_strat")(new_strat) | ||
| assert strategies._REGISTERED_STRATEGIES == {"new_strat": new_strat} | ||
|
|
There was a problem hiding this comment.
Just to check my understanding!
Here you first make sure that _REGISTERED_STRAT is empty. next you define a lambda function (easier than actually writing a user function) then you register it. But it seems kinda funny to do ._register("new_strat")(new_strat) - why the two ()() when _register just accpets (str)
There was a problem hiding this comment.
^^ Exactly correct!!
The reason for the two () calls is because register is a callable that returns a callable that actually does the registration. The registration is actually done during the second call. You can think of it as actually doing something like this
_tmp_fn_to_add_a_new_strategy_under_the_name_new_strategy = strategies._register("new_strat")
reveal_type(_tmp_fn_to_add_a_new_strategy_under_the_name_new_strategy)
# reveals: `t.Callable[[PermutationStrategyType], PermutationStrategyType]`
assert strategies._REGISTERED_STRATEGIES == {}
# Passes! Notice that nothing has been added yet
new_strat = lambda p, e, n: []
return_from_tmp_fn = _tmp_fn_to_add_a_new_strategy_under_the_name_new_strategy(new_strat)
assert return_from_tmp_fn is new_strat
# Passes! Notice that the function is returned unchanged
assert strategies._REGISTERED_STRATEGIES == {"new_strat": new_strat}
# Passes! Notice that the key was only added after
# `_tmp_fn_to_add_a_new_strategy_under_the_name_new_strategy` was calledAll of this is for the weird magic that let's us use this function like a decorator. This test is functionally testing this use case
stategies._register("new_strat")
def new_strat(p, e, n):
return []
amandarichardsonn
left a comment
amandarichardsonn
left a comment
There was a problem hiding this comment.
LGTM just 2 Qs
Creates a basic implementation for a `CompundEntity` base class that can be used to build a launchable containing multiple processes (`Job`s, `JobGroup`, etc) each with their own settings from a single `LaunchSettings` instance. This class is then subclassed to create an `Ensemble` class that mimics the original implementation: I takes in an `Application` (`Model`) and then maps settings/params/input files over the collection before creating the launchable jobs. [ committed by @MattToast ] [ reviewed by @amandarichardsonn ] --------- Co-authored-by: amandarichardsonn <[email protected]>
Creates a basic implementation for a `CompundEntity` base class that can be used to build a launchable containing multiple processes (`Job`s, `JobGroup`, etc) each with their own settings from a single `LaunchSettings` instance. This class is then subclassed to create an `Ensemble` class that mimics the original implementation: I takes in an `Application` (`Model`) and then maps settings/params/input files over the collection before creating the launchable jobs. [ committed by @MattToast ] [ reviewed by @amandarichardsonn ] --------- Co-authored-by: amandarichardsonn <[email protected]>
2 participants
Creates a basic implementation for a
CompundEntitybase class that can be used to build a launchable containing multiple processes (Jobs,JobGroup, etc) each with their own settings from a singleLaunchSettingsinstance.This class is then subclassed to create an
Ensembleclass that mimics the original implementation: I takes in anApplication(Model) and then maps settings/params/input files over the collection before creating the launchable jobs.