-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Training and scoring pipelines with AutoMLSearch #1913
Training and scoring pipelines with AutoMLSearch #1913
Conversation
a083332
to
435330a
Compare
Codecov Report
@@ Coverage Diff @@
## main #1913 +/- ##
=========================================
+ Coverage 100.0% 100.0% +0.1%
=========================================
Files 265 265
Lines 21737 21936 +199
=========================================
+ Hits 21731 21930 +199
Misses 6 6
Continue to review full report at Codecov.
|
evalml/automl/automl_search.py
Outdated
random_seed=self.random_seed) | ||
self._best_pipeline.fit(X_train, y_train) | ||
tune_binary_threshold(self._best_pipeline, self.objective, self.problem_type, X_threshold_tuning, y_threshold_tuning) | ||
best_pipeline = self._engine.train_pipeline(best_pipeline, self.X_train, self.y_train, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line will change once #1814 is merged. We'll have to subset to the ensemble indices before calling train_pipeline
3d0e31d
to
b49aee4
Compare
b49aee4
to
18bb3ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some questions and other tests that I think would be good to add! Definitely would be nice to discuss the implications of passing None
to train and score.
evalml/automl/engine/engine_base.py
Outdated
objective (ObjectiveBase): Objective used in threshold tuning. | ||
|
||
Returns: | ||
PipelineBase - trained pipeline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: can we do
"""
Returns:
pipeline (PipelineBase): Trained pipeline.
"""
Or something similar?
y (ww.DataTable, pd.DataFrame): Data to score on. | ||
objectives (list(ObjectiveBase), list(str)): Objectives to score on. | ||
Returns: | ||
Dict: Dict containining scores for all objectives for all pipelines. Keyed by pipeline name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: containing
evalml/automl/utils.py
Outdated
ValueError if any pipeline names are duplicated. | ||
""" | ||
seen_names = set([]) | ||
duplicate_names = set([]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason to do set([])
versus set()
? Just curious, I've only ever done set()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe set()
is better hehe I'll make the change.
|
||
@patch('evalml.pipelines.BinaryClassificationPipeline.score') | ||
@patch('evalml.pipelines.BinaryClassificationPipeline.fit') | ||
def test_train_batch_score_batch(mock_fit, mock_score, dummy_binary_pipeline_class, X_y_binary): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we try to train or score before AutoMLSearch has been run? Or if we try to score before we trained a pipeline batch? can we add test cov?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what happens if the user passes in a stacked ensemble pipeline (in both cases where ensembling runs and ensembling doesn't run)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great questions @bchen1116 ! train_batch
and score_batch
are adjacent to search and mainly help users leverage the underlying engine for fitting and scoring. In my mind it makes the most sense to run this after search with pipelines you get via get_pipeline
but I don't think we need to enforce that.
So to answer your questions:
- Should work as expected
- If you score a pipeline that hasn't been fit the exception will be in the log and the score will be nan for all objectives.
- Should work as expected, i.e. the pipeline is either fit/scored successfully.
I'll add coverage 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, sounds good to me!
evalml/automl/utils.py
Outdated
seen_names.add(pipeline.name) | ||
|
||
if duplicate_names: | ||
raise ValueError(f"All pipeline names must be unique. The names {', '.join(duplicate_names)} were repeated.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit pick, but can we add quotation marks around each name? Maybe something like
duplicates = "', '".join(duplicate_names)
f"All pipeline names must be unique. The names {duplicates} were repeated."
just for easier readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call
231d4dd
to
6fe721c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freddyaboulton thanks for the fixes! Everything looks great! I just left a comment on adding quotation marks around the name for clarity, since it seems like my original suggestion didn't work, but looks good otherwise!
class Pipeline2(dummy_binary_pipeline_class): | ||
custom_name = "My Pipeline" | ||
|
||
with pytest.raises(ValueError, match="All pipeline names must be unique. The names My Pipeline were repeated."): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sad, did the quotation marks not make it in. Can we add quotes to the pipeline names? Might need to do \"
or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
assert exception in caplog.text | ||
|
||
# Test training before search is run | ||
train_batch_and_check() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
e96ce50
to
3501ac5
Compare
docs/source/release_notes.rst
Outdated
@@ -10,6 +10,8 @@ Release Notes | |||
* Added utility method to create list of components from a list of ``DataCheckAction`` :pr:`1907` | |||
* Updated ``validate`` method to include a ``action`` key in returned dictionary for all ``DataCheck``and ``DataChecks`` :pr:`1916` | |||
* Aggregating the shap values for predictions that we know the provenance of, e.g. OHE, text, and date-time. :pr:`1901` | |||
* Added ``score_batch`` and ``train_batch`` methods to ``AutoMLSearch`` :pr:`1913` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think this should be train_pipelines
and score_pipelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're totally right 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this PR and I think it's really going to start to push some solid refactoring of the Engines and AutoMLSearch. I left a few comments, I think the only blocking ones are:
- Resolution of placement of EngineBase.tune_threshold() static method.
- Some docstring stuff.
- Pipeline cloning discussion and resolution.
evalml/automl/engine/engine_base.py
Outdated
""" | ||
X_threshold_tuning = None | ||
y_threshold_tuning = None | ||
if EngineBase.tune_threshold(pipeline, optimize_thresholds, objective): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I dunno, tune_threshold seems like it might better belong in the utils from whence tune_binary_threshold came. What do you think?
4464df1
to
b71837c
Compare
9511aa5
to
3437792
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Pull Request Description
Fixes #1729
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rst
to include this pull request by adding :pr:123
.