Capture full serialized metric representation when loading with skipped runners and metrics, in properties #2888
+101
−30
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ed runners and metrics, in properties Summary: **Context:** 1. **When an experiment is loaded with `skip_runners_and_metrics`** flag enabled, runners are simply omitted, but **metrics are not omitted, since their names and `lower_is_better` direction is needed in a few places during optimization** (e.g. in formatting and looking up the data). Instead of simply omitting them, we "stub" them as base `Metric` objects, which still have the name and the `lower_is_better` of the original metric. 2. Further, copies of experiment's metrics end up being saved on trials, unless `Experiment.immutable_search_space_and_opt_config` is true. **This is bad: we now have copies of the stubbed metrics saved on trials.** **What's done in this diff:** 1. We preserve the serialized representation of the original metric in the `Metric.properties` json blob (we don't need to decode it for this); 2. With 1) in place, we are able to, during re-encoding of the metrics, inject their correct representation (not stubbed) back into their SQA representation and save the metric in a way that matches its original form. **Is this beautiful code...** Most certainly not, but the problem is quite important and this does address it fully, functionality-wise. Addressing it more elegantly requires a full storage refactor (and a very clever one at that) or elimination of a) use of `Metric`-s throughout the optimization code or b) stopping storage of metric copies on GRs (which could be considered, but we corrently don't have another mechanism for keeping history of the opt. config). **But is this safe?** Yes; the "full SQA row" is only in the properties of the associated metrics, and attempting to make a change to the stubbed metric as if it's the non-stubbed version, wouldn't have worked anyway. **Does this apply to JSON serialization?** No, we don't do any stubbing there currently, although only because we don't ever need to reload from JSON with non-core registries. We can cross that bridge when we get there; the hope is that we can refactor storage before getting there at all. Differential Revision: D64212275
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D64212275 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Context:
skip_runners_and_metricsflag enabled, runners are simply omitted, but metrics are not omitted, since their names andlower_is_betterdirection is needed in a few places during optimization (e.g. in formatting and looking up the data). Instead of simply omitting them, we "stub" them as baseMetricobjects, which still have the name and thelower_is_betterof the original metric.Experiment.immutable_search_space_and_opt_configis true. This is bad: we now have copies of the stubbed metrics saved on trials.What's done in this diff:
Metric.propertiesjson blob (we don't need to decode it for this);Is this beautiful code... Most certainly not, but the problem is quite important and this does address it fully, functionality-wise. Addressing it more elegantly requires a full storage refactor (and a very clever one at that) or elimination of a) use of
Metric-s throughout the optimization code or b) stopping storage of metric copies on GRs (which could be considered, but we corrently don't have another mechanism for keeping history of the opt. config).But is this safe? Yes; the "full SQA row" is only in the properties of the associated metrics, and attempting to make a change to the stubbed metric as if it's the non-stubbed version, wouldn't have worked anyway.
Does this apply to JSON serialization? No, we don't do any stubbing there currently, although only because we don't ever need to reload from JSON with non-core registries. We can cross that bridge when we get there; the hope is that we can refactor storage before getting there at all.
Differential Revision: D64212275