wip split generationcriterion and transitioncriterion#4854
Closed
mgarrard wants to merge 1 commit intofacebook:mainfrom
Closed
wip split generationcriterion and transitioncriterion#4854mgarrard wants to merge 1 commit intofacebook:mainfrom
mgarrard wants to merge 1 commit intofacebook:mainfrom
Conversation
37a5978 to
f09803f
Compare
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Feb 5, 2026
Summary: **TLDR:** This diff splits TransitionCriterion into (1) TransitionCriterion and (2) GenerationBlockingCriterion. I think this makes sense to do because it *greatly* increases the conceptual clarity of the transition criterion. Some ways it does this include: 1. Removal of confusing dual purpose flags — block_transition_if_unmet and block_generation_if_met flags. Now transition criteria are inferred to block transition if unmet and generation criteria are inferred to raise informative errors if the criteria is met. 2. Each criterion contains less flags, and the flags are more directly intuitive. 3. With upcoming removal of special logic for online, we will need to add more generation blocking criteria (ie do we have an opt config), it is better to make this change before adding more criteria that will need to be migrated 4. It will allows the logic for transition and generation to be smoother — this diff keeps things ~= to exisiting logic as possible to minimize diff review overhead, but in subsequent diffs we can save fit time if we know we can’t generate from this node + can’t transition. It will also allow for some further clarification on generation/transition blocking logic that i think is contributing to the confusion of the file 5. i like that creating a new generation blocking criteria with a specific error to raise is easy and painless **Cons of this change:** - it’s a large change, sorry about that. - There is some duplication between TrialBased transition criterion and generation criterion. I explored using a Mixin here, but i find mixins tend to add unnecessary inheritance structures to reason about. **Most important files for review, in order of importance** 1. transition_criterion.py 2. generation_node.py 3. decoder.py 4. encoders.py 5. registery.py 6. generation_strategy_dispatch.py 7. generation_nodes.py 8. generation_strategy.py The remaining files are mainly trivial updates to tests **Note about backwards compatibility:** * This diff will directly decode legacy MaxGenerationParallelism as a generation blocking criterion called MaxGenrationParallelism * Historically, there are some instances of mintrials that have block_gen_if_met=True, this usually comes from enforce_num_trials=True. Now we call this MaxTrialsAwaitingData, and MinTrials is decoded as that. I am open to other, better names for this new criterion. **Other notes/potential improvements:** - we could split transition criterion, generation criterion, and utils into their own files. i kinda like them together, and if we do want to do this split i’d like to do it in a follow up to try to minimize an already v large blast radius Differential Revision: D92201085
f09803f to
2827896
Compare
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Feb 5, 2026
…erion (facebook#4854) Summary: **TLDR:** This diff splits TransitionCriterion into (1) TransitionCriterion and (2) GenerationBlockingCriterion. I think this makes sense to do because it *greatly* increases the conceptual clarity of the transition criterion. Some ways it does this include: 1. Removal of confusing dual purpose flags — block_transition_if_unmet and block_generation_if_met flags. Now transition criteria are inferred to block transition if unmet and generation criteria are inferred to raise informative errors if the criteria is met. 2. Each criterion contains less flags, and the flags are more directly intuitive. 3. With upcoming removal of special logic for online, we will need to add more generation blocking criteria (ie do we have an opt config), it is better to make this change before adding more criteria that will need to be migrated 4. It will allows the logic for transition and generation to be smoother — this diff keeps things ~= to exisiting logic as possible to minimize diff review overhead, but in subsequent diffs we can save fit time if we know we can’t generate from this node + can’t transition. It will also allow for some further clarification on generation/transition blocking logic that i think is contributing to the confusion of the file 5. i like that creating a new generation blocking criteria with a specific error to raise is easy and painless **Cons of this change:** - it’s a large change, sorry about that. - There is some duplication between TrialBased transition criterion and generation criterion. I explored using a Mixin here, but i find mixins tend to add unnecessary inheritance structures to reason about. **Most important files for review, in order of importance** 1. transition_criterion.py 2. generation_node.py 3. decoder.py 4. encoders.py 5. registery.py 6. generation_strategy_dispatch.py 7. generation_nodes.py 8. generation_strategy.py The remaining files are mainly trivial updates to tests **Note about backwards compatibility:** * This diff will directly decode legacy MaxGenerationParallelism as a generation blocking criterion called MaxGenrationParallelism * Historically, there are some instances of mintrials that have block_gen_if_met=True, this usually comes from enforce_num_trials=True. Now we call this MaxTrialsAwaitingData, and MinTrials is decoded as that. I am open to other, better names for this new criterion. **Other notes/potential improvements:** - we could split transition criterion, generation criterion, and utils into their own files. i kinda like them together, and if we do want to do this split i’d like to do it in a follow up to try to minimize an already v large blast radius Differential Revision: D92201085
2827896 to
211421b
Compare
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Feb 5, 2026
…erion (facebook#4854) Summary: **TLDR:** This diff splits TransitionCriterion into (1) TransitionCriterion and (2) GenerationBlockingCriterion. I think this makes sense to do because it *greatly* increases the conceptual clarity of the transition criterion. Some ways it does this include: 1. Removal of confusing dual purpose flags — block_transition_if_unmet and block_generation_if_met flags. Now transition criteria are inferred to block transition if unmet and generation criteria are inferred to raise informative errors if the criteria is met. 2. Each criterion contains less flags, and the flags are more directly intuitive. 3. With upcoming removal of special logic for online, we will need to add more generation blocking criteria (ie do we have an opt config), it is better to make this change before adding more criteria that will need to be migrated 4. It will allows the logic for transition and generation to be smoother — this diff keeps things ~= to exisiting logic as possible to minimize diff review overhead, but in subsequent diffs we can save fit time if we know we can’t generate from this node + can’t transition. It will also allow for some further clarification on generation/transition blocking logic that i think is contributing to the confusion of the file 5. i like that creating a new generation blocking criteria with a specific error to raise is easy and painless **Cons of this change:** - it’s a large change, sorry about that. - There is some duplication between TrialBased transition criterion and generation criterion. I explored using a Mixin here, but i find mixins tend to add unnecessary inheritance structures to reason about. **Most important files for review, in order of importance** 1. transition_criterion.py 2. generation_node.py 3. decoder.py 4. encoders.py 5. registery.py 6. generation_strategy_dispatch.py 7. generation_nodes.py 8. generation_strategy.py The remaining files are mainly trivial updates to tests **Note about backwards compatibility:** * This diff will directly decode legacy MaxGenerationParallelism as a generation blocking criterion called MaxGenrationParallelism * Historically, there are some instances of mintrials that have block_gen_if_met=True, this usually comes from enforce_num_trials=True. Now we call this MaxTrialsAwaitingData, and MinTrials is decoded as that. I am open to other, better names for this new criterion. **Other notes/potential improvements:** - we could split transition criterion, generation criterion, and utils into their own files. i kinda like them together, and if we do want to do this split i’d like to do it in a follow up to try to minimize an already v large blast radius Differential Revision: D92201085
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4854 +/- ##
========================================
Coverage 96.83% 96.83%
========================================
Files 596 596
Lines 63108 63244 +136
========================================
+ Hits 61109 61241 +132
- Misses 1999 2003 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
211421b to
4d4c50e
Compare
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Feb 24, 2026
…erion (facebook#4854) Summary: **TLDR:** This diff splits TransitionCriterion into (1) TransitionCriterion and (2) GenerationBlockingCriterion. I think this makes sense to do because it *greatly* increases the conceptual clarity of the transition criterion. Some ways it does this include: 1. Removal of confusing dual purpose flags — block_transition_if_unmet and block_generation_if_met flags. Now transition criteria are inferred to block transition if unmet and generation criteria are inferred to raise informative errors if the criteria is met. 2. Each criterion contains less flags, and the flags are more directly intuitive. 3. With upcoming removal of special logic for online, we will need to add more generation blocking criteria (ie do we have an opt config), it is better to make this change before adding more criteria that will need to be migrated 4. It will allows the logic for transition and generation to be smoother — this diff keeps things ~= to exisiting logic as possible to minimize diff review overhead, but in subsequent diffs we can save fit time if we know we can’t generate from this node + can’t transition. It will also allow for some further clarification on generation/transition blocking logic that i think is contributing to the confusion of the file 5. i like that creating a new generation blocking criteria with a specific error to raise is easy and painless **Cons of this change:** - it’s a large change, sorry about that. - There is some duplication between TrialBased transition criterion and generation criterion. I explored using a Mixin here, but i find mixins tend to add unnecessary inheritance structures to reason about. **Most important files for review, in order of importance** 1. transition_criterion.py 2. generation_node.py 3. decoder.py 4. encoders.py 5. registery.py 6. generation_strategy_dispatch.py 7. generation_nodes.py 8. generation_strategy.py The remaining files are mainly trivial updates to tests **Note about backwards compatibility:** * This diff will directly decode legacy MaxGenerationParallelism as a generation blocking criterion called MaxGenrationParallelism * Historically, there are some instances of mintrials that have block_gen_if_met=True, this usually comes from enforce_num_trials=True. Now we call this MaxTrialsAwaitingData, and MinTrials is decoded as that. I am open to other, better names for this new criterion. **Other notes/potential improvements:** - we could split transition criterion, generation criterion, and utils into their own files. i kinda like them together, and if we do want to do this split i’d like to do it in a follow up to try to minimize an already v large blast radius Reviewed By: lena-kashtelyan Differential Revision: D92201085
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Feb 24, 2026
…erion (facebook#4854) Summary: **TLDR:** This diff splits TransitionCriterion into (1) TransitionCriterion and (2) GenerationBlockingCriterion. I think this makes sense to do because it *greatly* increases the conceptual clarity of the transition criterion. Some ways it does this include: 1. Removal of confusing dual purpose flags — block_transition_if_unmet and block_generation_if_met flags. Now transition criteria are inferred to block transition if unmet and generation criteria are inferred to raise informative errors if the criteria is met. 2. Each criterion contains less flags, and the flags are more directly intuitive. 3. With upcoming removal of special logic for online, we will need to add more generation blocking criteria (ie do we have an opt config), it is better to make this change before adding more criteria that will need to be migrated 4. It will allows the logic for transition and generation to be smoother — this diff keeps things ~= to exisiting logic as possible to minimize diff review overhead, but in subsequent diffs we can save fit time if we know we can’t generate from this node + can’t transition. It will also allow for some further clarification on generation/transition blocking logic that i think is contributing to the confusion of the file 5. i like that creating a new generation blocking criteria with a specific error to raise is easy and painless **Cons of this change:** - it’s a large change, sorry about that. - There is some duplication between TrialBased transition criterion and generation criterion. I explored using a Mixin here, but i find mixins tend to add unnecessary inheritance structures to reason about. **Most important files for review, in order of importance** 1. transition_criterion.py 2. generation_node.py 3. decoder.py 4. encoders.py 5. registery.py 6. generation_strategy_dispatch.py 7. generation_nodes.py 8. generation_strategy.py The remaining files are mainly trivial updates to tests **Note about backwards compatibility:** * This diff will directly decode legacy MaxGenerationParallelism as a generation blocking criterion called MaxGenrationParallelism * Historically, there are some instances of mintrials that have block_gen_if_met=True, this usually comes from enforce_num_trials=True. Now we call this MaxTrialsAwaitingData, and MinTrials is decoded as that. I am open to other, better names for this new criterion. **Other notes/potential improvements:** - we could split transition criterion, generation criterion, and utils into their own files. i kinda like them together, and if we do want to do this split i’d like to do it in a follow up to try to minimize an already v large blast radius Reviewed By: lena-kashtelyan Differential Revision: D92201085
36dccfd to
9c67229
Compare
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Feb 24, 2026
…erion (facebook#4854) Summary: **TLDR:** This diff splits TransitionCriterion into (1) TransitionCriterion and (2) GenerationBlockingCriterion. I think this makes sense to do because it *greatly* increases the conceptual clarity of the transition criterion. Some ways it does this include: 1. Removal of confusing dual purpose flags — block_transition_if_unmet and block_generation_if_met flags. Now transition criteria are inferred to block transition if unmet and generation criteria are inferred to raise informative errors if the criteria is met. 2. Each criterion contains less flags, and the flags are more directly intuitive. 3. With upcoming removal of special logic for online, we will need to add more generation blocking criteria (ie do we have an opt config), it is better to make this change before adding more criteria that will need to be migrated 4. It will allows the logic for transition and generation to be smoother — this diff keeps things ~= to exisiting logic as possible to minimize diff review overhead, but in subsequent diffs we can save fit time if we know we can’t generate from this node + can’t transition. It will also allow for some further clarification on generation/transition blocking logic that i think is contributing to the confusion of the file 5. i like that creating a new generation blocking criteria with a specific error to raise is easy and painless **Cons of this change:** - it’s a large change, sorry about that. - There is some duplication between TrialBased transition criterion and generation criterion. I explored using a Mixin here, but i find mixins tend to add unnecessary inheritance structures to reason about. **Most important files for review, in order of importance** 1. transition_criterion.py 2. generation_node.py 3. decoder.py 4. encoders.py 5. registery.py 6. generation_strategy_dispatch.py 7. generation_nodes.py 8. generation_strategy.py The remaining files are mainly trivial updates to tests **Note about backwards compatibility:** * This diff will directly decode legacy MaxGenerationParallelism as a generation blocking criterion called MaxGenrationParallelism * Historically, there are some instances of mintrials that have block_gen_if_met=True, this usually comes from enforce_num_trials=True. Now we call this MaxTrialsAwaitingData, and MinTrials is decoded as that. I am open to other, better names for this new criterion. **Other notes/potential improvements:** - we could split transition criterion, generation criterion, and utils into their own files. i kinda like them together, and if we do want to do this split i’d like to do it in a follow up to try to minimize an already v large blast radius Reviewed By: lena-kashtelyan Differential Revision: D92201085
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Feb 24, 2026
…erion (facebook#4854) Summary: Pull Request resolved: facebook#4854 **TLDR:** This diff splits TransitionCriterion into (1) TransitionCriterion and (2) GenerationBlockingCriterion. I think this makes sense to do because it *greatly* increases the conceptual clarity of the transition criterion. Some ways it does this include: 1. Removal of confusing dual purpose flags — block_transition_if_unmet and block_generation_if_met flags. Now transition criteria are inferred to block transition if unmet and generation criteria are inferred to raise informative errors if the criteria is met. 2. Each criterion contains less flags, and the flags are more directly intuitive. 3. With upcoming removal of special logic for online, we will need to add more generation blocking criteria (ie do we have an opt config), it is better to make this change before adding more criteria that will need to be migrated 4. It will allows the logic for transition and generation to be smoother — this diff keeps things ~= to exisiting logic as possible to minimize diff review overhead, but in subsequent diffs we can save fit time if we know we can’t generate from this node + can’t transition. It will also allow for some further clarification on generation/transition blocking logic that i think is contributing to the confusion of the file 5. i like that creating a new generation blocking criteria with a specific error to raise is easy and painless **Cons of this change:** - it’s a large change, sorry about that. - There is some duplication between TrialBased transition criterion and generation criterion. I explored using a Mixin here, but i find mixins tend to add unnecessary inheritance structures to reason about. **Most important files for review, in order of importance** 1. transition_criterion.py 2. generation_node.py 3. decoder.py 4. encoders.py 5. registery.py 6. generation_strategy_dispatch.py 7. generation_nodes.py 8. generation_strategy.py The remaining files are mainly trivial updates to tests **Note about backwards compatibility:** * This diff will directly decode legacy MaxGenerationParallelism as a generation blocking criterion called MaxGenrationParallelism * Historically, there are some instances of mintrials that have block_gen_if_met=True, this usually comes from enforce_num_trials=True. Now we call this MaxTrialsAwaitingData, and MinTrials is decoded as that. I am open to other, better names for this new criterion. **Other notes/potential improvements:** - we could split transition criterion, generation criterion, and utils into their own files. i kinda like them together, and if we do want to do this split i’d like to do it in a follow up to try to minimize an already v large blast radius Reviewed By: lena-kashtelyan Differential Revision: D92201085
f672739 to
6174c31
Compare
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Feb 24, 2026
…erion (facebook#4854) Summary: Pull Request resolved: facebook#4854 **TLDR:** This diff splits TransitionCriterion into (1) TransitionCriterion and (2) GenerationBlockingCriterion. I think this makes sense to do because it *greatly* increases the conceptual clarity of the transition criterion. Some ways it does this include: 1. Removal of confusing dual purpose flags — block_transition_if_unmet and block_generation_if_met flags. Now transition criteria are inferred to block transition if unmet and generation criteria are inferred to raise informative errors if the criteria is met. 2. Each criterion contains less flags, and the flags are more directly intuitive. 3. With upcoming removal of special logic for online, we will need to add more generation blocking criteria (ie do we have an opt config), it is better to make this change before adding more criteria that will need to be migrated 4. It will allows the logic for transition and generation to be smoother — this diff keeps things ~= to exisiting logic as possible to minimize diff review overhead, but in subsequent diffs we can save fit time if we know we can’t generate from this node + can’t transition. It will also allow for some further clarification on generation/transition blocking logic that i think is contributing to the confusion of the file 5. i like that creating a new generation blocking criteria with a specific error to raise is easy and painless **Cons of this change:** - it’s a large change, sorry about that. - There is some duplication between TrialBased transition criterion and generation criterion. I explored using a Mixin here, but i find mixins tend to add unnecessary inheritance structures to reason about. **Most important files for review, in order of importance** 1. transition_criterion.py 2. generation_node.py 3. decoder.py 4. encoders.py 5. registery.py 6. generation_strategy_dispatch.py 7. generation_nodes.py 8. generation_strategy.py The remaining files are mainly trivial updates to tests **Note about backwards compatibility:** * This diff will directly decode legacy MaxGenerationParallelism as a generation blocking criterion called MaxGenrationParallelism * Historically, there are some instances of mintrials that have block_gen_if_met=True, this usually comes from enforce_num_trials=True. Now we call this MaxTrialsAwaitingData, and MinTrials is decoded as that. I am open to other, better names for this new criterion. **Other notes/potential improvements:** - we could split transition criterion, generation criterion, and utils into their own files. i kinda like them together, and if we do want to do this split i’d like to do it in a follow up to try to minimize an already v large blast radius Reviewed By: lena-kashtelyan Differential Revision: D92201085
6174c31 to
838c843
Compare
mgarrard
added a commit
to mgarrard/Ax
that referenced
this pull request
Feb 25, 2026
…erion (facebook#4854) Summary: **TLDR:** This diff splits TransitionCriterion into (1) TransitionCriterion and (2) GenerationBlockingCriterion. I think this makes sense to do because it *greatly* increases the conceptual clarity of the transition criterion. Some ways it does this include: 1. Removal of confusing dual purpose flags — block_transition_if_unmet and block_generation_if_met flags. Now transition criteria are inferred to block transition if unmet and generation criteria are inferred to raise informative errors if the criteria is met. 2. Each criterion contains less flags, and the flags are more directly intuitive. 3. With upcoming removal of special logic for online, we will need to add more generation blocking criteria (ie do we have an opt config), it is better to make this change before adding more criteria that will need to be migrated 4. It will allows the logic for transition and generation to be smoother — this diff keeps things ~= to exisiting logic as possible to minimize diff review overhead, but in subsequent diffs we can save fit time if we know we can’t generate from this node + can’t transition. It will also allow for some further clarification on generation/transition blocking logic that i think is contributing to the confusion of the file 5. i like that creating a new generation blocking criteria with a specific error to raise is easy and painless **Cons of this change:** - it’s a large change, sorry about that. - There is some duplication between TrialBased transition criterion and generation criterion. I explored using a Mixin here, but i find mixins tend to add unnecessary inheritance structures to reason about. **Most important files for review, in order of importance** 1. transition_criterion.py 2. generation_node.py 3. decoder.py 4. encoders.py 5. registery.py 6. generation_strategy_dispatch.py 7. generation_nodes.py 8. generation_strategy.py The remaining files are mainly trivial updates to tests **Note about backwards compatibility:** * This diff will directly decode legacy MaxGenerationParallelism as a generation blocking criterion called MaxGenrationParallelism * Historically, there are some instances of mintrials that have block_gen_if_met=True, this usually comes from enforce_num_trials=True. Now we call this MaxTrialsAwaitingData, and MinTrials is decoded as that. I am open to other, better names for this new criterion. **Other notes/potential improvements:** - we could split transition criterion, generation criterion, and utils into their own files. i kinda like them together, and if we do want to do this split i’d like to do it in a follow up to try to minimize an already v large blast radius Reviewed By: lena-kashtelyan Differential Revision: D92201085
…erion (facebook#4854) Summary: **TLDR:** This diff splits TransitionCriterion into (1) TransitionCriterion and (2) GenerationBlockingCriterion. I think this makes sense to do because it *greatly* increases the conceptual clarity of the transition criterion. Some ways it does this include: 1. Removal of confusing dual purpose flags — block_transition_if_unmet and block_generation_if_met flags. Now transition criteria are inferred to block transition if unmet and generation criteria are inferred to raise informative errors if the criteria is met. 2. Each criterion contains less flags, and the flags are more directly intuitive. 3. With upcoming removal of special logic for online, we will need to add more generation blocking criteria (ie do we have an opt config), it is better to make this change before adding more criteria that will need to be migrated 4. It will allows the logic for transition and generation to be smoother — this diff keeps things ~= to exisiting logic as possible to minimize diff review overhead, but in subsequent diffs we can save fit time if we know we can’t generate from this node + can’t transition. It will also allow for some further clarification on generation/transition blocking logic that i think is contributing to the confusion of the file 5. i like that creating a new generation blocking criteria with a specific error to raise is easy and painless **Cons of this change:** - it’s a large change, sorry about that. - There is some duplication between TrialBased transition criterion and generation criterion. I explored using a Mixin here, but i find mixins tend to add unnecessary inheritance structures to reason about. **Most important files for review, in order of importance** 1. transition_criterion.py 2. generation_node.py 3. decoder.py 4. encoders.py 5. registery.py 6. generation_strategy_dispatch.py 7. generation_nodes.py 8. generation_strategy.py The remaining files are mainly trivial updates to tests **Note about backwards compatibility:** * This diff will directly decode legacy MaxGenerationParallelism as a generation blocking criterion called MaxGenrationParallelism * Historically, there are some instances of mintrials that have block_gen_if_met=True, this usually comes from enforce_num_trials=True. Now we call this MaxTrialsAwaitingData, and MinTrials is decoded as that. I am open to other, better names for this new criterion. **Other notes/potential improvements:** - we could split transition criterion, generation criterion, and utils into their own files. i kinda like them together, and if we do want to do this split i’d like to do it in a follow up to try to minimize an already v large blast radius Reviewed By: lena-kashtelyan Differential Revision: D92201085
838c843 to
7d0a6f1
Compare
|
This pull request has been merged in debdf00. |
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: wip
Differential Revision: D92201085