Fix InSampleUniformGenerator selecting arms from CANDIDATE trials#5109
Closed
ItsMrLin wants to merge 1 commit into
Closed
Fix InSampleUniformGenerator selecting arms from CANDIDATE trials#5109ItsMrLin wants to merge 1 commit into
ItsMrLin wants to merge 1 commit into
Conversation
Summary: When LILO labeling generates pairwise comparisons, `InSampleUniformGenerator` selects 2 arms uniformly at random from `generated_points` built by `RandomAdapter`. Previously, this pool included arms from CANDIDATE trials (which have no observed data) via two sources: 1. `arms_by_signature_for_deduplication` (all non-FAILED arms) 2. `pending_observations` (CANDIDATE/STAGED/RUNNING arms appended for dedup) When a CANDIDATE trial arm was selected, `LILOPairwiseMetric` could not find source metric data for it, causing `fetch_data` to raise an `ExceptionGroup`. **Fix:** In `RandomAdapter._gen()`, when the generator is `InSampleUniformGenerator`: - Filter `arms_to_deduplicate` to only arms from `expecting_data` trials (COMPLETED, EARLY_STOPPED, RUNNING) - Skip appending `pending_observations` (which re-adds CANDIDATE arms) This is correct for all current and foreseeable use cases of `InSampleUniformGenerator`. The generator is used in two scenarios: LILO labeling (active) and potentially bandit candidate generation (not currently using it). In both cases, we need arms with observed data -- selecting an arm from a CANDIDATE or STAGED trial that has never been evaluated is never meaningful. Excluding pending observations is similarly safe: pending points exist to prevent regular generators (Sobol, BO) from re-suggesting in-flight arms, but for in-sample selection the entire point is to pick from already-observed arms. The change is gated behind an `isinstance` check -- no effect on other generators (Sobol, Uniform, etc.). Reviewed By: saitcakmak Differential Revision: D98627073
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5109 +/- ##
=======================================
Coverage 96.41% 96.41%
=======================================
Files 613 613
Lines 68106 68131 +25
=======================================
+ Hits 65663 65688 +25
Misses 2443 2443 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This pull request has been merged in 218bf55. |
This file contains hidden or 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
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:
When LILO labeling generates pairwise comparisons,
InSampleUniformGeneratorselects 2 arms uniformly at random fromgenerated_pointsbuilt byRandomAdapter. Previously, this pool included arms from CANDIDATE trials (which have no observed data) via two sources:arms_by_signature_for_deduplication(all non-FAILED arms)pending_observations(CANDIDATE/STAGED/RUNNING arms appended for dedup)When a CANDIDATE trial arm was selected,
LILOPairwiseMetriccould not find source metric data for it, causingfetch_datato raise anExceptionGroup.Fix: In
RandomAdapter._gen(), when the generator isInSampleUniformGenerator:arms_to_deduplicateto only arms fromexpecting_datatrials (COMPLETED, EARLY_STOPPED, RUNNING)pending_observations(which re-adds CANDIDATE arms)This is correct for all current and foreseeable use cases of
InSampleUniformGenerator. The generator is used in two scenarios: LILO labeling (active) and potentially bandit candidate generation (not currently using it). In both cases, we need arms with observed data -- selecting an arm from a CANDIDATE or STAGED trial that has never been evaluated is never meaningful. Excluding pending observations is similarly safe: pending points exist to prevent regular generators (Sobol, BO) from re-suggesting in-flight arms, but for in-sample selection the entire point is to pick from already-observed arms.The change is gated behind an
isinstancecheck -- no effect on other generators (Sobol, Uniform, etc.).Reviewed By: saitcakmak
Differential Revision: D98627073