feat: Allow for customized extra results to be written to results.csv.zip #612

mseeger · 2023-04-01T17:07:16Z

At the moment, we write time-stamped results to results.csv.zip (a dataframe), and the serialized tuner to tuner.dill.
This PR allows to (optionally) write extra results to results.csv.zip. These are appended as new columns.

An example use case is given.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

.github/workflows/integ-tests.yml

mseeger · 2023-04-01T17:09:30Z

benchmarking/commons/hpo_main_local.py

    return tuner_kwargs


 def start_benchmark_local_backend(
    configuration: ConfigDict,
    methods: MethodDefinitions,
    benchmark_definitions: RealBenchmarkDefinitions,
-    post_processing: Optional[PostProcessingType] = None,
+    final_results: Optional[FinalResultsComposer] = None,


Replaces post_processing. Instead of running some code at the end, we extract some extra results which are stored. This is more useful.

Are there any known uses of customers using post_processing?

I agree that final_results is a great new feature -- just wondering before we remove post_processing if there are any use-cases that will break from the removal.

Especially since this is in benchmarking, I think it's very low-risk to update.

Good point. I don't think anybody except me uses this right now.

benchmarking/commons/utils.py

codecov · 2023-04-01T17:14:31Z

Codecov Report

Patch coverage: 59.48% and project coverage change: -0.02 ⚠️

Comparison is base (62f1b8c) 65.11% compared to head (cabf154) 65.10%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #612      +/-   ##
==========================================
- Coverage   65.11%   65.10%   -0.02%     
==========================================
  Files         362      363       +1     
  Lines       26500    26532      +32     
==========================================
+ Hits        17256    17274      +18     
- Misses       9244     9258      +14

Impacted Files	Coverage Δ
benchmarking/commons/hpo_main_common.py	`47.22% <ø> (-0.96%)`	⬇️
benchmarking/commons/hpo_main_local.py	`0.00% <0.00%> (ø)`
benchmarking/commons/hpo_main_sagemaker.py	`0.00% <0.00%> (ø)`
benchmarking/commons/hpo_main_simulator.py	`0.00% <0.00%> (ø)`
benchmarking/commons/utils.py	`0.00% <0.00%> (ø)`
benchmarking/nursery/benchmark_dyhpo/hpo_main.py	`0.00% <0.00%> (ø)`
syne_tune/experiments.py	`0.00% <0.00%> (ø)`
...ptimizer/schedulers/searchers/searcher_callback.py	`0.00% <0.00%> (ø)`
syne_tune/tuner_callback.py	`100.00% <ø> (ø)`
...izer/schedulers/searchers/dyhpo/hyperband_dyhpo.py	`94.44% <66.66%> (+0.24%)`	⬆️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

syne_tune/constants.py

syne_tune/results_callback.py

wesk · 2023-04-03T11:43:27Z

syne_tune/results_callback.py

+    def _write_final_results(self):
+        if self._final_results_composer is not None:
+            final_results = self._final_results_composer(self._tuner)
+            if final_results is not None:
+                path = self._tuner.tuner_path / ST_FINAL_RESULTS_FILENAME
+                dump_json_with_numpy(final_results, path)


How does this new method relate to this existing way of getting the final hyperparameters: https://github.com/awslabs/syne-tune/blob/main/examples/launch_plot_results.py#L69

tuning_experiment = load_experiment(tuner.name) print(tuning_experiment) print(f"best result found: {tuning_experiment.best_config()}")

Is this _final_results_composer needed if it's already possible to call .best_config()? Could we not re-use what .best_config() is doing?

Final results are not best parameters. They can be anything. In the example I have here, they are statistics recorded by the scheduler, so nothing that would even be in results.csv.zip.

In the next PR, I am using this to record performance statistics for checkpoint removal. Again, this is not stored in results.csv.

These final results are optional. If you can obtain your answer from the results.csv, you should do that. But often, this is not possible.

Did you consider adding the ability to optionally augment the information in results.csv.zip to include the additional optional information that you want to extract? (Rather than creating a new separate output file, which might confuse users by creating two competing sources-of-truth.)

With the current implementation, I can foresee scenarios where people expect to be able to extract the best hyperparameters from final-results.json, however they don't find them there. It may become confusing to understand when to look in final-results.json versus results.csv.zip.

Or, if we choose not to combine the files, maybe renaming it to something like extra-metadata.json, so that it's clear that the primary output of tuning, the hyperparameters, will not be included in the file.

You have a good point, I'll do that

wesk · 2023-04-03T11:45:25Z

tst/schedulers/test_searchers.py

@@ -66,7 +66,7 @@

 # Does not contain ASHABORE, because >10 secs on CI
 # TODO: Dig more, why is ASHABORE more expensive than BORE here?
-@pytest.mark.timeout(10)
+@pytest.mark.timeout(12)


Looks like writing the files to disk makes the test run longer? Is this with intermediate checkpoints enabled, or is this just writing final output to disk?

I am not sure. I just increased this because I had this test failing. I do not think this has anything to do with the change here, to be honest. The change here is not activated in this test, right? Final results are only written when you activate the feature.

We can bump the limit, but if it was passing reliably before this PR, and starts timing out with this PR, then it would be useful to know why.

Not necessarily a blocker but something interesting

mseeger · 2023-04-03T13:48:31Z

I will rework this, so information is appended to the results.csv dataframe. Thanks Wes, this is indeed a better solution.

…csv.zip

mseeger · 2023-04-03T19:41:57Z

OK, this is totally rewritten. The extra results are now appended to the default ones, by extending the dataframe by additional columns. This has the advantage of not needing another file, and of getting time-stamped results.

The disadvantage is we cannot write out final results of complex structure (e.g., lists or dicts). But that is OK.

wesk · 2023-04-04T14:39:44Z

examples/launch_height_extra_results.py

+if __name__ == "__main__":
+    logging.getLogger().setLevel(logging.INFO)
+
+    random_seed = 31415927


nice seed choice 🥧

wesk

Very intuitive, looks good!

mseeger requested a review from wesk April 1, 2023 17:07

mseeger commented Apr 1, 2023

View reviewed changes

.github/workflows/integ-tests.yml Outdated Show resolved Hide resolved

mseeger commented Apr 1, 2023

View reviewed changes

benchmarking/commons/utils.py Show resolved Hide resolved

mseeger force-pushed the results_at_end branch from 2921c1b to 7535547 Compare April 1, 2023 20:22

wesk reviewed Apr 3, 2023

View reviewed changes

syne_tune/constants.py Show resolved Hide resolved

wesk reviewed Apr 3, 2023

View reviewed changes

syne_tune/results_callback.py Outdated Show resolved Hide resolved

wesk reviewed Apr 3, 2023

View reviewed changes

feature: Allow for customized extra results to be written to results.…

a3328b8

…csv.zip

mseeger force-pushed the results_at_end branch from c5ac9eb to a3328b8 Compare April 3, 2023 19:30

mseeger changed the title ~~feature: New result file (optional) to be written at the end of tuning~~ feature: Allow for customized extra results to be written to resultscsv.zip Apr 3, 2023

mseeger changed the title ~~feature: Allow for customized extra results to be written to resultscsv.zip~~ feature: Allow for customized extra results to be written to results.csv.zip Apr 3, 2023

Fix

7268a25

wesk reviewed Apr 4, 2023

View reviewed changes

wesk approved these changes Apr 4, 2023

View reviewed changes

Merge branch 'main' into results_at_end

cabf154

mseeger merged commit 9185aa3 into main Apr 4, 2023

mseeger deleted the results_at_end branch April 4, 2023 15:01

wesk added the feature label Apr 11, 2023

wesk changed the title ~~feature: Allow for customized extra results to be written to results.csv.zip~~ feat: Allow for customized extra results to be written to results.csv.zip Apr 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow for customized extra results to be written to results.csv.zip #612

feat: Allow for customized extra results to be written to results.csv.zip #612

mseeger commented Apr 1, 2023 •

edited

Loading

mseeger Apr 1, 2023

wesk Apr 3, 2023

mseeger Apr 3, 2023

codecov bot commented Apr 1, 2023 •

edited

Loading

wesk Apr 3, 2023

mseeger Apr 3, 2023

wesk Apr 3, 2023

mseeger Apr 3, 2023

wesk Apr 3, 2023

mseeger Apr 3, 2023

wesk Apr 3, 2023

mseeger commented Apr 3, 2023

mseeger commented Apr 3, 2023

wesk Apr 4, 2023

wesk left a comment

feat: Allow for customized extra results to be written to results.csv.zip #612

feat: Allow for customized extra results to be written to results.csv.zip #612

Conversation

mseeger commented Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 1, 2023 • edited Loading

Codecov Report

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

mseeger commented Apr 3, 2023

mseeger commented Apr 3, 2023

Choose a reason for hiding this comment

wesk left a comment

Choose a reason for hiding this comment

mseeger commented Apr 1, 2023 •

edited

Loading

codecov bot commented Apr 1, 2023 •

edited

Loading