Skip to content

Conversation

@ltiao
Copy link
Contributor

@ltiao ltiao commented Nov 24, 2025

Summary:
This diff adds a check_safe boolean parameter to BaseEarlyStoppingStrategy and all its child classes to control whether the _is_harmful safety check is applied when making early stopping decisions.

When check_safe=False (default), the safety check is bypassed and early stopping decisions from _should_stop_trials_early are applied directly. When check_safe=True, the _is_harmful check gates early stopping to prevent potentially harmful stopping decisions.

The parameter is added to:

  • BaseEarlyStoppingStrategy.__init__
  • ModelBasedEarlyStoppingStrategy.__init__
  • PercentileEarlyStoppingStrategy.__init__
  • ThresholdEarlyStoppingStrategy.__init__

All child classes default to check_safe=False to maintain backward compatibility while allowing opt in to safety checks as needed.

Differential Revision: D87492602

@meta-cla meta-cla bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Nov 24, 2025
@meta-codesync
Copy link

meta-codesync bot commented Nov 24, 2025

@ltiao has exported this pull request. If you are a Meta employee, you can view the originating Diff in D87492602.

ltiao added a commit to ltiao/Ax that referenced this pull request Nov 25, 2025
Summary:

This diff adds a `check_safe` boolean parameter to `BaseEarlyStoppingStrategy` and all its child classes to control whether the `_is_harmful` safety check is applied when making early stopping decisions.

When `check_safe=False` (default), the safety check is bypassed and early stopping decisions from `_should_stop_trials_early` are applied directly. When `check_safe=True`, the `_is_harmful` check gates early stopping to prevent potentially harmful stopping decisions.

The parameter is added to:
- `BaseEarlyStoppingStrategy.__init__`
- `ModelBasedEarlyStoppingStrategy.__init__`
- `PercentileEarlyStoppingStrategy.__init__`
- `ThresholdEarlyStoppingStrategy.__init__`

All child classes default to `check_safe=False` to maintain backward compatibility while allowing opt in to safety checks as needed.

Differential Revision: D87492602
@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2025

Codecov Report

❌ Patch coverage is 92.30769% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.49%. Comparing base (e26c9d7) to head (dba8eea).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ax/early_stopping/strategies/base.py 77.77% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4579      +/-   ##
==========================================
- Coverage   96.50%   96.49%   -0.02%     
==========================================
  Files         557      557              
  Lines       57359    57387      +28     
==========================================
+ Hits        55356    55377      +21     
- Misses       2003     2010       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ltiao added a commit to ltiao/Ax that referenced this pull request Nov 26, 2025
Summary:

This diff adds a `check_safe` boolean parameter to `BaseEarlyStoppingStrategy` and all its child classes to control whether the `_is_harmful` safety check is applied when making early stopping decisions.

When `check_safe=False` (default), the safety check is bypassed and early stopping decisions from `_should_stop_trials_early` are applied directly. When `check_safe=True`, the `_is_harmful` check gates early stopping to prevent potentially harmful stopping decisions.

The parameter is added to:
- `BaseEarlyStoppingStrategy.__init__`
- `ModelBasedEarlyStoppingStrategy.__init__`
- `PercentileEarlyStoppingStrategy.__init__`
- `ThresholdEarlyStoppingStrategy.__init__`

All child classes default to `check_safe=False` to maintain backward compatibility while allowing opt in to safety checks as needed.

Differential Revision: D87492602
… Naming (facebook#4578)

Summary:

**Context:**

This diff refactors the early stopping strategies codebase to eliminate code duplication and improve code clarity through better naming conventions.

- Eliminated ~50 lines of duplicate data preparation logic
- Established single source of truth for data preparation in base class
- Improved code maintainability and consistency
- No functional changes - purely refactoring

**Changes:**

1. **Moved `_prepare_aligned_data()` to base class** (`BaseEarlyStoppingStrategy`)
   * Previously duplicated in `PercentileEarlyStoppingStrategy` and `MultiObjectiveEarlyStoppingStrategy` (and also in future concrete implements of `_is_harmful`). Now a reusable helper method available to all strategies
2. **Renamed method for clarity**
   * `_check_validity_and_get_data()` → `_lookup_and_validate_data()` (emphasizes "lookup" terminology consistent with Ax codebase conventions; more accurately describes the method's purpose)
3. **Improved parameter naming across all strategies**
   * `df`, `df_raw` → `wide_df`, `long_df`
   * Clearly distinguishes between wide and long format dataframes
   * Updated in all early stopping strategy implementations and tests
4. **Updated documentation**

Reviewed By: saitcakmak

Differential Revision: D87573286
Summary:

This diff adds a `check_safe` boolean parameter to `BaseEarlyStoppingStrategy` and all its child classes to control whether the `_is_harmful` safety check is applied when making early stopping decisions.

When `check_safe=False` (default), the safety check is bypassed and early stopping decisions from `_should_stop_trials_early` are applied directly. When `check_safe=True`, the `_is_harmful` check gates early stopping to prevent potentially harmful stopping decisions.

The parameter is added to:
- `BaseEarlyStoppingStrategy.__init__`
- `ModelBasedEarlyStoppingStrategy.__init__`
- `PercentileEarlyStoppingStrategy.__init__`
- `ThresholdEarlyStoppingStrategy.__init__`

All child classes default to `check_safe=False` to maintain backward compatibility while allowing opt in to safety checks as needed.

Reviewed By: saitcakmak

Differential Revision: D87492602
ltiao added a commit to ltiao/Ax that referenced this pull request Nov 26, 2025
Summary:

This diff adds a `check_safe` boolean parameter to `BaseEarlyStoppingStrategy` and all its child classes to control whether the `_is_harmful` safety check is applied when making early stopping decisions.

When `check_safe=False` (default), the safety check is bypassed and early stopping decisions from `_should_stop_trials_early` are applied directly. When `check_safe=True`, the `_is_harmful` check gates early stopping to prevent potentially harmful stopping decisions.

The parameter is added to:
- `BaseEarlyStoppingStrategy.__init__`
- `ModelBasedEarlyStoppingStrategy.__init__`
- `PercentileEarlyStoppingStrategy.__init__`
- `ThresholdEarlyStoppingStrategy.__init__`

All child classes default to `check_safe=False` to maintain backward compatibility while allowing opt in to safety checks as needed.

Reviewed By: saitcakmak

Differential Revision: D87492602
ltiao added a commit to ltiao/Ax that referenced this pull request Nov 26, 2025
Summary:

This diff adds a `check_safe` boolean parameter to `BaseEarlyStoppingStrategy` and all its child classes to control whether the `_is_harmful` safety check is applied when making early stopping decisions.

When `check_safe=False` (default), the safety check is bypassed and early stopping decisions from `_should_stop_trials_early` are applied directly. When `check_safe=True`, the `_is_harmful` check gates early stopping to prevent potentially harmful stopping decisions.

The parameter is added to:
- `BaseEarlyStoppingStrategy.__init__`
- `ModelBasedEarlyStoppingStrategy.__init__`
- `PercentileEarlyStoppingStrategy.__init__`
- `ThresholdEarlyStoppingStrategy.__init__`

All child classes default to `check_safe=False` to maintain backward compatibility while allowing opt in to safety checks as needed.

Reviewed By: saitcakmak

Differential Revision: D87492602
@meta-codesync meta-codesync bot closed this in c66160c Nov 26, 2025
@meta-codesync
Copy link

meta-codesync bot commented Nov 26, 2025

This pull request has been merged in c66160c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Do not delete this pull request or issue due to inactivity. fb-exported Merged meta-exported

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants