Add log to file of parameters #353

al-rigazzi · 2023-08-31T18:42:27Z

This PR adds logging features for parameter values when generating entities or ensembles.

The log on the standard out is now optional and gives information about values set for each parameter in each model. The same information, combined with the affected files is now written to a file, param_settings.txt which is located in the experiment path.

The content of the file is of this type:

Model name: dir_test_0
File name                   Parameters
--------------------------  ---------------
dir_test/dir_test_0/in.atm  Name      Value
                            ------  -------
                            THERMO       10
                            STEPS        10

Model name: dir_test_1
File name                   Parameters
--------------------------  ---------------
dir_test/dir_test_1/in.atm  Name      Value
                            ------  -------
                            THERMO       10
                            STEPS        20

Model name: dir_test_2
File name                   Parameters
--------------------------  ---------------
dir_test/dir_test_2/in.atm  Name      Value
                            ------  -------
                            THERMO       20
                            STEPS        10

Model name: dir_test_3
File name                   Parameters
--------------------------  ---------------
dir_test/dir_test_3/in.atm  Name      Value
                            ------  -------
                            THERMO       20
                            STEPS        20

codecov · 2023-08-31T19:03:02Z

Codecov Report

Merging #353 (dc937f2) into develop (e1a5783) will decrease coverage by 0.10%.
Report is 6 commits behind head on develop.
The diff coverage is 98.03%.

@@             Coverage Diff             @@
##           develop     #353      +/-   ##
===========================================
- Coverage    88.54%   88.44%   -0.10%     
===========================================
  Files           59       58       -1     
  Lines         3580     3584       +4     
===========================================
  Hits          3170     3170              
- Misses         410      414       +4

Files Changed	Coverage
smartsim/experiment.py	`50.00%`
smartsim/_core/generation/generator.py	`100.00%`
smartsim/_core/generation/modelwriter.py	`100.00%`
smartsim/entity/model.py	`100.00%`

…og-ensemble-params

MattToast

Haven't had a chance to fully review yet, but I did have one thought I wanted to make sure I voiced

MattToast · 2023-09-01T17:36:27Z

smartsim/_core/generation/generator.py

+                level=self.log_level,
+                msg=f"Configured model {entity.name} with params {used_params_str}",
+            )
+            with open(self.log_file, mode="a", encoding="utf-8") as logfile:


Quick sanity check for my understanding:

If I have this setup to an experiment:

exp = Experiment("my_exp") rs1 = exp.create_run_settings(...) ens1 = exp.create_ensemble("my_ens_1", run_settings=rs1, params={"SPAM": [...], "EGGS": [...]}) ens1.attach_generator_files(to_configure=[...]) rs2 = exp.create_run_settings(...) ens2 = exp.create_ensemble("my_ens_2", run_settings=rs2, params={"FOO": [...], "BAR": [...]}) ens2.attach_generator_files(to_configure=[...]) exp.generate(ens1, ens2)

This will generate one single param_settings.txt in the experiment root, correct? Would it be worth the effort to make multiple param_settings.txts instead of/in addition to this file in the ensemble roots?

I am in favor of the additional ones, but I'd like to keep the experiment-wide one in the root, for a quicker inspection.

Added them.

…og-ensemble-params

MattToast

Looks great!! I have some minor knit picks and a couple of questions, but otherwise this looks about ready to go!

smartsim/_core/generation/generator.py

MattToast · 2023-09-19T00:22:34Z

smartsim/_core/generation/generator.py

+            log_entry = f"Model name: {entity.name}" + "\n" + file_table + "\n\n"
+            with open(self.log_file, mode="a", encoding="utf-8") as logfile:
+                logfile.write(log_entry)
+            with open(join(entity.path, "param_settings.txt"),


Incredibly wild corner case to consider here:

exp = Experiment(...) rs = exp.create_run_settings(...) ens = experiment.create_ensemble(params=..., run_settings=rs) ens.attach_generator_files(to_configure=['./param_settings.txt']) exp.generate(ens)

will actually overwrite the generated file. I know I was the one who originally asked for this file, but don't like the idea of "banning" users from attaching files of a certain name. Given this corner case, on second thought, I think we should probably only generate the top level param_settings.txt

If we do decide to keep the per-entity param_settings.txt, we should instead explicitly raise an error if a user attempts to copy/symlink/generate a param_settings.txt

OK, this needs an in-depth discussion.
The top-level param_settings.txt file gets overwritten every time a generation is called. But as we allow partial generation of experiments, this scenario leads to an unexpected result:

exp = Experiment(...) rs1 = exp.create_run_settings(...) rs2 = exp.create_run_settings(...) ens1 = experiment.create_ensemble(params=..., run_settings=rs1) ens1.attach_generator_files(...) ens2 = experiment.create_ensemble(params=..., run_settings=rs2) ens2.attach_generator_files(...) exp.generate(ens1) # the top-level file now contains info about ens1 parameters exp.generate(ens2) # the top-level file now contains info about ens2 parameters, but ens1 info was overwritten

This happens because we are overwriting the file instead of appending.
But if we appended, then there would be no way to overwrite the file if the experiment is re-generated, meaning we would keep appending for every run (even if the user chose overwrite=True, because we cannot interpret that correctly again due to partial generation being possible).

I suggest we:

select a more unique name, such as smartsim_params.txt which should be unlikely to be chosen by users

we make it a reserved file name and raise ValueError if the user really wants that file name.

Thoughts?

I would be okay with the purposed suggestion above!

Given the reasoning for the replace-not-append decision, I'm inclined to agree with the need for, and am once again in favor of, the per-entity parameter reporting file.

The chances of a name conflict on a smartsim_params.txt file seems incredibly unlikely, and if we can raise an exception about a potential file conflict prior to exp.start(ens), that should save users a lot of debugging headache.

Added exception at generation level!

tests/test_generator.py

tests/test_modelwriter.py

MattToast

LGTM!!

Add log to file of parameters and test

4abf7a5

al-rigazzi self-assigned this Aug 31, 2023

al-rigazzi requested review from ashao and MattToast August 31, 2023 18:43

al-rigazzi added 2 commits August 31, 2023 14:44

Remove trailing space

d23cea6

Merge branch 'develop' of https://github.com/CrayLabs/SmartSim into l…

d44f620

…og-ensemble-params

al-rigazzi added type: feature Issues that include feature request or feature idea type: usability Issues related to ease of use labels Sep 5, 2023

MattToast reviewed Sep 6, 2023

View reviewed changes

MattToast self-requested a review September 6, 2023 23:49

al-rigazzi added 4 commits September 7, 2023 19:15

Clean up test files

184694a

Fix modelwriter tests

d8a1639

Set global log file open mode to w

887abeb

Lint

159dcf2

al-rigazzi changed the title ~~Add log to file of parameters and test~~ Add log to file of parameters Sep 14, 2023

Merge branch 'develop' of https://github.com/CrayLabs/SmartSim into l…

3e8917a

…og-ensemble-params

MattToast requested changes Sep 19, 2023

View reviewed changes

al-rigazzi added 3 commits September 22, 2023 17:39

Address reviewer's comments

2e7b2df

Circumvent type issue

b3c78da

Remove unused import

dc937f2

al-rigazzi requested a review from MattToast September 23, 2023 00:23

MattToast approved these changes Sep 25, 2023

View reviewed changes

al-rigazzi merged commit 7d4cc31 into CrayLabs:develop Sep 28, 2023
10 checks passed

al-rigazzi deleted the log-ensemble-params branch September 28, 2023 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add log to file of parameters #353

Add log to file of parameters #353

al-rigazzi commented Aug 31, 2023 •

edited

Loading

codecov bot commented Aug 31, 2023 •

edited

Loading

MattToast left a comment

MattToast Sep 1, 2023

al-rigazzi Sep 7, 2023

al-rigazzi Sep 7, 2023

MattToast left a comment

MattToast Sep 19, 2023 •

edited

Loading

al-rigazzi Sep 19, 2023

MattToast Sep 19, 2023

al-rigazzi Sep 28, 2023

MattToast left a comment

Add log to file of parameters #353

Add log to file of parameters #353

Conversation

al-rigazzi commented Aug 31, 2023 • edited Loading

codecov bot commented Aug 31, 2023 • edited Loading

Codecov Report

MattToast left a comment

Choose a reason for hiding this comment

MattToast Sep 1, 2023

Choose a reason for hiding this comment

al-rigazzi Sep 7, 2023

Choose a reason for hiding this comment

al-rigazzi Sep 7, 2023

Choose a reason for hiding this comment

MattToast left a comment

Choose a reason for hiding this comment

MattToast Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

al-rigazzi Sep 19, 2023

Choose a reason for hiding this comment

MattToast Sep 19, 2023

Choose a reason for hiding this comment

al-rigazzi Sep 28, 2023

Choose a reason for hiding this comment

MattToast left a comment

Choose a reason for hiding this comment

al-rigazzi commented Aug 31, 2023 •

edited

Loading

codecov bot commented Aug 31, 2023 •

edited

Loading

MattToast Sep 19, 2023 •

edited

Loading