Ensemble.files refactor #746

amandarichardsonn · 2024-10-15T20:26:07Z

Attaching files to an Application and Ensemble has been refactored.

There is now two operations files, an operations.py file that holds the tools to attach files to an Application and an ensemble_operations.py file that holds the tools to attach files to an Ensemble. These two operation files are covered in individual test files: test_operations.py and test_ensemble_operations.py.

Additionally, building the file operation commands in the Generator has been refactored to support the removal of EntityFiles and replacement of the operations.FileSysOpSet. Tests for generator.py have been updated in test_generator.py.

Additionally, the function Ensemble._create_applications has been reworked to support the new Ensemble.files type.

Files to investigate:
smartsim/generation/operations/ensemble_operations.py
smartsim/generation/operations/operations.py
smartsim/generation/operations/utils/helpers.py
smartsim/builders/ensemble.py
smartsim/entity/application.py

codecov · 2024-10-15T20:29:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.79%. Comparing base (cce16e6) to head (f4e4cdb).
Report is 19 commits behind head on smartsim-refactor.

Additional details and impacted files

@@                  Coverage Diff                  @@
##           smartsim-refactor     #746      +/-   ##
=====================================================
+ Coverage              40.45%   47.79%   +7.33%     
=====================================================
  Files                    110      109       -1     
  Lines                   7326     6576     -750     
=====================================================
+ Hits                    2964     3143     +179     
+ Misses                  4362     3433     -929

Files with missing lines	Coverage Δ
..._core/generation/operations/ensemble_operations.py	`100.00% <100.00%> (ø)`
smartsim/builders/ensemble.py	`99.15% <100.00%> (ø)`
smartsim/entity/application.py	`90.00% <ø> (ø)`

... and 11 files with indirect coverage changes

amandarichardsonn · 2024-10-15T22:40:12Z

Question for @juliaputko ! In application.py you placed the TODO about 2 months ago in the remove dead attrs in application PR, is this safe to remove now?

# TODO: Remove this supression when we strip fileds/functionality
#       (run-settings/batch_settings/params_as_args/etc)!
# pylint: disable-next=too-many-public-methods

juliaputko

lookin great! You can also add a '''match="error message"''' to pytest to make sure you are failing on the right valueerror/type error that you are expecting. Up to you!

juliaputko · 2024-10-17T21:26:04Z

smartsim/_core/generation/operations/ensemble_operations.py

+        :param tag: Tag to use for find and replacement
+        """
+        self.operations.append(
+            EnsembleConfigureOperation(src, file_parameters, dest, tag)


can we change the order of these so that it matches the configure method?

also I dont know if file_parameters is the best name for what is going in? This is a mapping of the items that need to be found and replaced right?

juliaputko · 2024-10-17T21:31:59Z

smartsim/builders/ensemble.py

@@ -424,8 +431,8 @@ def build_jobs(self, settings: LaunchSettings) -> tuple[Job, ...]:
        :raises TypeError: if the ids argument is not type LaunchSettings
        :raises ValueError: if the LaunchSettings provided are empty
        """
-        if not isinstance(settings, LaunchSettings):
-            raise TypeError("ids argument was not of type LaunchSettings")
+        # if not isinstance(settings, LaunchSettings):


remove commented lines?

juliaputko · 2024-10-17T21:32:26Z

tests/test_ensemble_operations.py

+
+pytestmark = pytest.mark.group_a
+
+# TODO missing test for _filter


address todo?

ankona

Looks good. As an outsider to the core refactor, I went through and focused on the docstrings and how clear they were to me. Added some suggestions that may/may not be included based on your opinions.

ankona · 2024-10-28T20:29:32Z

smartsim/_core/generation/operations/ensemble_operations.py

+
+
+class EnsembleCopyOperation(EnsembleGenerationProtocol):
+    """Ensemble Copy Operation"""


Consider describing the functionality.

""" A file generation operation used to specify parameters for copying a file into a location accessible to the Ensemble. """

better description than my three words eep! will change

ankona · 2024-10-28T20:31:31Z

smartsim/_core/generation/operations/ensemble_operations.py

+        """
+        check_src_and_dest_path(src, dest)
+        self.src = src
+        """Path to source"""


Consider being explicit Path to the file that will be copied

ankona · 2024-10-28T20:37:08Z

smartsim/_core/generation/operations/ensemble_operations.py

+    def __init__(
+        self, src: pathlib.Path, dest: t.Optional[pathlib.Path] = None
+    ) -> None:
+        """Initialize a EnsembleCopyOperation object


What is the result if the destination path is not supplied. Should this behavior be specified in this docstring?

yes I will specify

ankona · 2024-10-28T20:37:36Z

smartsim/_core/generation/operations/ensemble_operations.py

+        """Initialize a EnsembleSymlinkOperation object
+
+        :param src: Path to source
+        :param dest: Path to destination


What is the result if the destination path is not supplied. Should this behavior be specified in this docstring?

ankona · 2024-10-28T20:38:32Z

smartsim/_core/generation/operations/ensemble_operations.py

+        """
+        check_src_and_dest_path(src, dest)
+        self.src = src
+        """Path to source"""


Consider being explicit: Path to the file that will be symlink'ed

ankona · 2024-10-28T21:04:49Z

smartsim/builders/ensemble.py

 class Ensemble(entity.CompoundEntity):
-    """An Ensemble is a builder class that parameterizes the creation of multiple
+    """An Ensemble is a builder class to parameterize the creation of multiple


omit "an ensemble..."

Suggested change

"""An Ensemble is a builder class to parameterize the creation of multiple

"""A builder class used to create parameterized versions of an Application.

if we say "i don't know what a builder class is" after reading the description, do we need more information?

ankona · 2024-10-28T21:08:45Z

smartsim/builders/ensemble.py

+        file: EnsembleConfigureOperation,
+        permutation_strategy: strategies.PermutationStrategyType,
+    ) -> list[FileSet]:
+        """Generate all possible permutations of file parameters using the provided strategy,


Suggested change

"""Generate all possible permutations of file parameters using the provided strategy,

"""Generate the permutations of file parameters using the provided strategy,

no need to say all possible since that is implied in using the provided strategy - i wouldn't expect you to generate only some of the possibilities or you'd give me a new strategy that does that...

ankona · 2024-10-28T21:12:48Z

smartsim/builders/ensemble.py

+        """Generate all possible permutations of file parameters using the provided strategy,
+        and create FileSet objects.
+
+        This method applies the provided permutation strategy to the file's parameters,


avoid repeating the same thing as the first sentence of the docstring.

File and execution argument parameters are distributed using the supplied strategy. When a permutation limit is specified, the .... fill in unknown behavior here ... <maybe>permutations returned are randomly sampled from the complete set of permutations</maybe>

ankona · 2024-10-28T21:24:42Z

smartsim/builders/ensemble.py

+    def _attach_files(
+        self, app: Application, file_set_tuple: tuple[FileSet, ...]
+    ) -> None:
+        """Attach files to an Application.


nitpick: again, we're attaching the operations, not the files themselves

all nitpicks welcome always!

ankona · 2024-10-28T21:25:41Z

smartsim/builders/ensemble.py

-        if not isinstance(settings, LaunchSettings):
-            raise TypeError("ids argument was not of type LaunchSettings")
+        # if not isinstance(settings, LaunchSettings):
+        #     raise TypeError("ids argument was not of type LaunchSettings")
        apps = self._create_applications()
        if not apps:
            raise ValueError("There are no members as part of this ensemble")


Consider "The ensemble has no members."

amandarichardsonn added 30 commits September 30, 2024 09:11

initial push

3cac1d1

operations module close to finish

b6b5022

styling and mypy and partial test passing

6e88a40

saving spot

c93c85d

very ugly testing

3866bbd

adding configuration tests

b2c5421

pushing updates

e221b43

remove unused print statements

69f7866

fix typos

143ff4b

updates to PR

cdd82a4

initial push

56eb31c

partial address matts comments

83e015a

addressed Matts comments, exclude Qs

70dd2a0

update to ensemble

b9c0137

Merge remote-tracking branch 'upstream/smartsim-refactor' into gen

44a3b18

updates

3a2012e

Merge remote-tracking branch 'origin/gen' into builder_op

006a430

updates

6c2d908

styling up to date

f87d7cd

Merge remote-tracking branch 'origin/gen' into builder_op

5de9628

file rename

a7960f1

remove unused fixtures from operations

a6391cf

Merge remote-tracking branch 'origin/gen' into builder_op

96c21bd

small updates but pushing bc issue in gen branch

e937f0d

adding docstrings

36e8801

pushing for an idea in gen

1fe7719

adding default tag

b1dbbb3

Merge remote-tracking branch 'origin/gen' into builder_op

f42e208

testing operations complete

8c7a767

ensemble op test complete

06f530d

amandarichardsonn added 4 commits October 14, 2024 20:33

pushing to switch branches

8a9a577

pushing test updates

e88f226

Merge remote-tracking branch 'origin/gen' into builder_op

d60df2d

pushing to PR

d632588

amandarichardsonn added 3 commits October 15, 2024 15:37

remove unused EntityFile imports

27aa710

Merge remote-tracking branch 'origin/gen' into builder_op

2f9838f

remove EntityFiles

7bc70f2

amandarichardsonn added 4 commits October 15, 2024 15:42

remove dead fixture

b71871e

changing types fro t.List to list

87372e1

Merge remote-tracking branch 'origin/gen' into builder_op

a8965bd

changing t.List to list

bd2ac50

amandarichardsonn marked this pull request as draft October 15, 2024 23:12

amandarichardsonn added 2 commits October 16, 2024 14:15

address one comment from Matt

6198a14

Merge remote-tracking branch 'origin/gen' into builder_op

8ca13c3

amandarichardsonn changed the title ~~Ensemble File refactor~~ Ensemble.files refactor Oct 16, 2024

amandarichardsonn marked this pull request as ready for review October 16, 2024 22:22

amandarichardsonn added 2 commits October 16, 2024 15:58

failing

c039680

Merge branch 'smartsim-refactor' into builder_op

f4e4cdb

amandarichardsonn requested review from mellis13 and juliaputko October 16, 2024 23:07

juliaputko reviewed Oct 17, 2024

View reviewed changes

ankona reviewed Oct 28, 2024

View reviewed changes

MattToast self-requested a review October 31, 2024 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensemble.files refactor #746

Ensemble.files refactor #746

amandarichardsonn commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

amandarichardsonn commented Oct 15, 2024 •

edited

Loading

juliaputko left a comment

juliaputko Oct 17, 2024

juliaputko Oct 17, 2024

juliaputko Oct 17, 2024

juliaputko Oct 17, 2024

ankona left a comment

ankona Oct 28, 2024

amandarichardsonn Oct 28, 2024

ankona Oct 28, 2024

ankona Oct 28, 2024

amandarichardsonn Oct 28, 2024

ankona Oct 28, 2024

ankona Oct 28, 2024

ankona Oct 28, 2024

ankona Oct 28, 2024

ankona Oct 28, 2024

ankona Oct 28, 2024

ankona Oct 28, 2024

amandarichardsonn Oct 28, 2024

ankona Oct 28, 2024


		pytestmark = pytest.mark.group_a

		# TODO missing test for _filter



		class EnsembleCopyOperation(EnsembleGenerationProtocol):
		"""Ensemble Copy Operation"""

	"""An Ensemble is a builder class to parameterize the creation of multiple
	"""A builder class used to create parameterized versions of an Application.

	"""Generate all possible permutations of file parameters using the provided strategy,
	"""Generate the permutations of file parameters using the provided strategy,

Ensemble.files refactor #746

Are you sure you want to change the base?

Ensemble.files refactor #746

Conversation

amandarichardsonn commented Oct 15, 2024 • edited Loading

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

amandarichardsonn commented Oct 15, 2024 • edited Loading

juliaputko 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

ankona 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

amandarichardsonn commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

amandarichardsonn commented Oct 15, 2024 •

edited

Loading