Do not deduplicate against FAILED in GenerationStrategy/Node #2488
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
Summary: While working on D57478100, I noticed that CANDIDATE trials were not being included in `pending_features` but they were being used for deduplication checks. This diff resolves this inconsistency by adding CANDIDATE trials to `pending_features`. Also updates `get_pending_observation_features_based_on_trial_status` to avoid repeating same computations for each metric on the experiment. Differential Revision: D57514290
…k#2488) Summary: We have an implicit rule that the `ABANDONED` trials failed deterministically and should not be generated again, while `FAILED` trials are unknown and may be generated again if the model chooses to do so. This rule is encoded in the utilities used to extract `pending_observations` from experiment while generating candidates, which counts `CANDIDATE` (as of D57514290), `STAGED`, `RUNNING` & `ABANDONED` trials. The `EARLY_STOPPED` and `COMPLETED` trials have data associated with them, which was a similar effect to `pending_observations` in discouraging the models from generating them again. Notably, `FAILED` trials do not fall into either category, thus they are fair game for the model to generate again, if it determines that to be the optimal next step. This conflicts with the current deduplication against `experiment.arms_by_signature` which includes all arms on the experiment regardless of the trial status. To make the behavior consistent, this diff adds a new `arms_by_signature_for_deduplication` property on the `Experiment` that excludes arms attached to the `FAILED` trials. Differential Revision: D57478100
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D57478100 |
saitcakmak
force-pushed
the
export-D57478100
branch
from
0e4ba5b
to
d7eba32
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57478100 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2488 +/- ##
=======================================
Coverage 95.28% 95.28%
=======================================
Files 485 485
Lines 47400 47410 +10
=======================================
+ Hits 45163 45174 +11
+ Misses 2237 2236 -1 ☔ View full report in Codecov by Sentry. |
|
This pull request has been merged in 4100602. |
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:
We have an implicit rule that the
ABANDONEDtrials failed deterministically and should not be generated again, whileFAILEDtrials are unknown and may be generated again if the model chooses to do so. This rule is encoded in the utilities used to extractpending_observationsfrom experiment while generating candidates, which countsCANDIDATE(as of D57514290),STAGED,RUNNING&ABANDONEDtrials. TheEARLY_STOPPEDandCOMPLETEDtrials have data associated with them, which was a similar effect topending_observationsin discouraging the models from generating them again. Notably,FAILEDtrials do not fall into either category, thus they are fair game for the model to generate again, if it determines that to be the optimal next step.This conflicts with the current deduplication against
experiment.arms_by_signaturewhich includes all arms on the experiment regardless of the trial status. To make the behavior consistent, this diff adds a newarms_by_signature_for_deduplicationproperty on theExperimentthat excludes arms attached to theFAILEDtrials.Differential Revision: D57478100