Skip to content
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

Engine API #1838

Merged
merged 2 commits into from Feb 19, 2021
Merged

Engine API #1838

merged 2 commits into from Feb 19, 2021

Conversation

dsherry
Copy link
Contributor

@dsherry dsherry commented Feb 12, 2021

Fix #1296

Changes

  • Add EngineBase base class to define engine interface, and SequentialEngine which preserves existing behavior, evaluates pipelines in series
  • Define _pre_evaluation_callback for logging and _post_evaluation_callback to add evaluation results to AutoMLSearch
  • Move AutoMLSearch. _compute_cv_scores to EngineBase.train_and_score_pipeline as a staticmethod
  • Refactored the AutoMLSearch binary threshold tuning logic into tune_binary_threshold so it could be reused. Fixed bug where all bin class pipelines were getting a threshold of 0.5 -- now will get None as was the original intent.
  • Tweak criterion for doing bin threshold tuning to apply to timeseries binary classification as well.
  • Now that AutoMLSearch constructor expects data, move much of the search setup logic into the constructor to simplify the behavior.
  • Have AutoMLAlgorithm.add_result raise error if pipeline is unrecognized
  • Update and simplify keyboard interrupt code; no intended change in user behavior
  • Rename _check_stopping_condition to _should_continue
  • Add traceback to the end of log_error_callback, which I believe partially handles No stack trace visible in debug logย #1778 (@bchen1116 heads up). This was helpful to me when debugging so I just went ahead and did it.
  • Fixed some test bugs
    • In test_automl.py, CustomClassificationObjective wasn't subclassed from BinaryClassificationObjective, same for regression
    • Many tests were mocking pipeline score but not providing a return value, causing scores to be nan

Future:

  • Pass a config object to engine classes' constructor instead of AutoMLSearch instance. I couldn't easily do this today because of the logging callback -- it expects a reference to AutoMLSearch to add the error to the results structure
  • When we implement parallel evaluation, we'll have to decide what to do with the early stopping parameters, as those won't map over easily to the parallel batched evaluation format.
  • The changes in this PR are not threadsafe. The calls to the should_continue/pre/post callbacks access and modify AutoMLSearch state without any sort of locking or atomicity. This is fine for this PR, but when we implement parallel evaluation, we'll need to address this.
  • Add ability to accept an EngineBase subclass as input to AutoMLSearch, rather than creating internally. This would require thought about how to pass parameters and create the instance. We'd probably wanna switch to using a config object first for AutoMLSearch

Big credit to @christopherbunn for providing the initial implementation this PR is based on! ๐Ÿ™

@dsherry dsherry force-pushed the 1296_search_engine_api_rebase branch 2 times, most recently from f481e52 to fd4055b Compare February 12, 2021 15:10
@dsherry dsherry mentioned this pull request Feb 12, 2021
@dsherry dsherry force-pushed the 1296_search_engine_api_rebase branch 3 times, most recently from d9343ce to 4e03cef Compare February 16, 2021 05:39
@codecov
Copy link

codecov bot commented Feb 16, 2021

Codecov Report

Merging #1838 (e9eaea2) into main (99dd43e) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             main    #1838     +/-   ##
=========================================
+ Coverage   100.0%   100.0%   +0.1%     
=========================================
  Files         255      260      +5     
  Lines       20644    20832    +188     
=========================================
+ Hits        20636    20826    +190     
+ Misses          8        6      -2     
Impacted Files Coverage ฮ”
evalml/pipelines/pipeline_base.py 100.0% <รธ> (รธ)
evalml/automl/__init__.py 100.0% <100.0%> (รธ)
evalml/automl/automl_algorithm/automl_algorithm.py 100.0% <100.0%> (รธ)
evalml/automl/automl_search.py 100.0% <100.0%> (+0.4%) โฌ†๏ธ
evalml/automl/callbacks.py 100.0% <100.0%> (รธ)
evalml/automl/engine/__init__.py 100.0% <100.0%> (รธ)
evalml/automl/engine/engine_base.py 100.0% <100.0%> (รธ)
evalml/automl/engine/sequential_engine.py 100.0% <100.0%> (รธ)
evalml/automl/utils.py 100.0% <100.0%> (รธ)
evalml/objectives/objective_base.py 100.0% <100.0%> (รธ)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
ฮ” = absolute <relative> (impact), รธ = not affected, ? = missing data
Powered by Codecov. Last update 99dd43e...e9eaea2. Read the comment docs.

evaluation_result = EngineBase.train_and_score_pipeline(pipeline, self.automl, self.X_train, self.y_train)
new_pipeline_ids.append(self._post_evaluation_callback(pipeline, evaluation_result))
pipelines.pop(0)
return new_pipeline_ids
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A weakness of this code: if train_and_score_pipeline or one of the callbacks errors out, the whole while loop would halt.

We use the error callback inside train_and_score_pipeline to handle errors during pipeline evaluation, so this isn't a big concern. But if the data splitter errors, it would move up the stack. I think that behavior is acceptable for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dumb question: can we wrap train_score in a try/except and just continue/alert on exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not dumb! You are correct, adding proper error handling now is the responsible thing to do. I was being lazy. Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a close look at this. I still agree that we should update the way we do error handling. But I'd like us to do it in a separate PR. There's a decent amount to consider. This PR doesn't change the existing error handling pattern; we will have to in order to complete the parallel work.

@dsherry dsherry force-pushed the 1296_search_engine_api_rebase branch from 2b83ab5 to 4328c60 Compare February 16, 2021 20:06
Copy link
Collaborator

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Looks like a lot, but looks good.

evalml/automl/automl_search.py Outdated Show resolved Hide resolved
evalml/automl/automl_search.py Show resolved Hide resolved
evalml/automl/engine/engine_base.py Outdated Show resolved Hide resolved
evaluation_result = EngineBase.train_and_score_pipeline(pipeline, self.automl, self.X_train, self.y_train)
new_pipeline_ids.append(self._post_evaluation_callback(pipeline, evaluation_result))
pipelines.pop(0)
return new_pipeline_ids
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dumb question: can we wrap train_score in a try/except and just continue/alert on exception?

evalml/automl/engine/sequential_engine.py Outdated Show resolved Hide resolved
evalml/automl/utils.py Show resolved Hide resolved
evalml/tests/automl_tests/test_engine_base.py Show resolved Hide resolved
evalml/tests/automl_tests/test_engine_base.py Show resolved Hide resolved
evalml/tests/automl_tests/test_sequential_engine.py Outdated Show resolved Hide resolved
Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big PR! Tests and implementation looks good, but had a few cleanup suggestions. Had a question about add_to_rankings. It seems like the performance of it changed compared to main, because when I run

from evalml import AutoMLSearch
from evalml.demos import load_breast_cancer
from evalml.pipelines import BinaryClassificationPipeline

class NewPipeline(BinaryClassificationPipeline):
    component_graph = ['Imputer', 'LightGBM Classifier']

X, y = load_breast_cancer()
automl = AutoMLSearch(X_train=X, y_train=y, problem_type='binary', objective='f1')
automl.search()    
automl.add_to_rankings(NewPipeline({}))
automl.full_rankings

I get
image
whereas running on main doesn't give me this same error.

evalml/automl/automl_algorithm/iterative_algorithm.py Outdated Show resolved Hide resolved
evalml/automl/engine/engine_base.py Outdated Show resolved Hide resolved
evalml/automl/engine/engine_base.py Outdated Show resolved Hide resolved
evalml/automl/engine/engine_base.py Outdated Show resolved Hide resolved
evalml/tests/automl_tests/test_engine_base.py Show resolved Hide resolved
evalml/tests/automl_tests/test_automl.py Show resolved Hide resolved
evalml/tests/automl_tests/test_automl.py Outdated Show resolved Hide resolved
@dsherry
Copy link
Contributor Author

dsherry commented Feb 17, 2021

@bchen1116 thanks for your comments!

That example you included is fantastic. You are correct. The way this code is laid out in this PR currently, it would prevent users from adding pipelines to the rankings leaderboard if they were not initially included in allowed_pipelines. After reflecting on your comments here and in the PR, I agree, there's no reason to add this limitation. I think I got confused, because its kinda weird that the existing code always tries to add pipelines to the automl algorithm tuners--it doesn't make sense to do that if no tuner exists!

I just updated the code to preserve the existing behavior for add_to_rankings (no exception thrown), and also tried to clean up the implementation. AutoMLAlgorithm.add_result will still raise an exception, because that seems like a helpful thing for that API to do, but the AutoMLSearch results callback swallows the exception and continues.

Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments! The implementation looks good to me, just left a few nit-picky comments but nothing too pressing here.

Do you think it'd be useful to run perf tests on this to ensure that the new search algo performances don't differ too much from what we currently have on main?

evalml/automl/automl_search.py Show resolved Hide resolved
evalml/automl/engine/engine_base.py Show resolved Hide resolved
Returns:
list (int): a list of the new pipeline IDs which were created by the AutoML search.
"""
if self.X_train is None or self.y_train is None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're checking that self.X_train, self.y_train aren't None, should we also do this for the AutoMLSearch object? If that's None, this call should also fail
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. I suppose it can't hurt to add that. Since there's only one call site at the moment, and if we didn't set automl search there all the tests will fail, I will hold off on adding that right now.

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is an incredible PR with a lot of great refactoring ๐Ÿ‘๏ธ ๐Ÿ‘๏ธ

evalml/automl/utils.py Outdated Show resolved Hide resolved
evalml/automl/utils.py Show resolved Hide resolved
evalml/tests/automl_tests/test_automl.py Show resolved Hide resolved
evalml/automl/utils.py Show resolved Hide resolved
evalml/tests/automl_tests/test_automl.py Show resolved Hide resolved
christopherbunn and others added 2 commits February 18, 2021 21:41
Removed extra evaluate_pipeline mock and set unprocessed pipelines to batch.

Added test to check for unprocessed pipelines

Move tune_binary_threshold to helper, and fix some ifs

Put optional max on the number of pipelines generated in a particular automl batch

Docstring fix

First pass

Lint

Reorder code to minimize diff

Remove unused code

Changelog

A couple small fixes

Fix name

Send the right callback

Fix callback

May not need callback helper

Show traceback during automl search errors

A couple fixes

more use of is_binary / is_multiclass

Fix tests

Lint

Remove unused tests

Have AutoMLAlgorithm.add_result raise exception if there's a nonexistent pipeline name provided

Reword

Add another test

Simplify err msg

Fix more tests

Move logic from search up to __init__ to avoid errors with add_to_rankings

Lint

Fix import

Attempt to fix test

Lint

Fix add_to_rankings and test

more test fixes

Fix another test

Fix var name

Only print out best pipeline at end if one exists!

Fix keyboard interrupt. make it easier to use pdb with

Have engine return new pipeline IDs. Fix the way the all-nan check works

update all-nan tests

Fix another test

Fix a test case in huge parametrize

Fix test: use appropriate objective subclass so that can_optimize_threshold is defined for binclass objective

Lint

More test fixes

Update classification tests

Fix a few tests, and stopiteration

Lint

Fix regression tests

Fix more regression tests

Rename 'engines' package to 'engine'

Refactor, start updating engine tests

More test updates

Remove unneeded test

No need to return pipeline from helper

Remove vestigial arg

Add test coverage for train_and_score_pipelines

Lint

Unit test coverage for engine

Add patience/tolerance back in

Lint

Back out unused max_num_pipelines change

Remove unused docstring

Remove single-call helper

Move test files

Docstrings

Make helper method static

Fix mocking in test for py3.7

Amend previous

Log cleanup

Fix prev

Fix conditional

Remove set_data helper, simplify API

Docs nit-pick

Back out another unnecessary change

Simplify loop

Add test coverage for tune_binary_threshold and is_defined_for_problem_type; allow str input for is_defined_for_problem_type

Add assert

Codecov

Mock input directly

Try to add pipelines to results, but if the automl algo wasn't initialized with them, ignore the error

Little changes
@dsherry dsherry force-pushed the 1296_search_engine_api_rebase branch from 956d32f to e9eaea2 Compare February 19, 2021 03:30
@dsherry
Copy link
Contributor Author

dsherry commented Feb 19, 2021

Perf test results here. Flat as expected!

@dsherry dsherry merged commit b1b3961 into main Feb 19, 2021
@dsherry dsherry deleted the 1296_search_engine_api_rebase branch February 19, 2021 04:18
@chukarsten chukarsten mentioned this pull request Feb 23, 2021
@dsherry dsherry mentioned this pull request Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Search Engine API
5 participants