Remove `pipeline_parameters` and `custom_hyperparameters` and replace with `search_parameters` by bchen1116 · Pull Request #3373 · alteryx/evalml

bchen1116 · 2022-03-14T19:26:34Z

Design doc in confluence

codecov · 2022-03-14T19:32:37Z

Codecov Report

Merging #3373 (2847634) into main (da8f266) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #3373     +/-   ##
=======================================
- Coverage   99.7%   99.6%   -0.0%     
=======================================
  Files        329     329             
  Lines      32405   32380     -25     
=======================================
- Hits       32276   32249     -27     
- Misses       129     131      +2

Impacted Files	Coverage Δ
...sts/test_automl_search_classification_iterative.py	`100.0% <ø> (ø)`
evalml/automl/automl_algorithm/automl_algorithm.py	`100.0% <100.0%> (ø)`
...valml/automl/automl_algorithm/default_algorithm.py	`100.0% <100.0%> (ø)`
...lml/automl/automl_algorithm/iterative_algorithm.py	`97.4% <100.0%> (-1.0%)`	⬇️
evalml/automl/automl_search.py	`99.6% <100.0%> (-0.1%)`	⬇️
...ts/automl_tests/parallel_tests/test_automl_dask.py	`96.3% <100.0%> (ø)`
evalml/tests/automl_tests/test_automl.py	`99.5% <100.0%> (+0.1%)`	⬆️
evalml/tests/automl_tests/test_automl_algorithm.py	`98.6% <100.0%> (+0.5%)`	⬆️
...ts/automl_tests/test_automl_iterative_algorithm.py	`100.0% <100.0%> (ø)`
.../automl_tests/test_automl_search_classification.py	`96.5% <100.0%> (ø)`
... and 6 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 da8f266...2847634. Read the comment docs.

eccabay

Just have some nitpicky doc comments for now, I'll come back and do a full review later!

docs/source/release_notes.rst

docs/source/user_guide/automl.ipynb

freddyaboulton · 2022-03-15T19:29:32Z

evalml/automl/automl_algorithm/automl_algorithm.py

+        for (
+            name,
+            component_instance,
+        ) in pipeline.component_graph.component_instances.items():


@bchen1116 This code block is doing two things:

Getting random values from the skopt spaces so that the parameters used in the first batch are in the space the tuner is tuning over

Making sure the the _pipeline_parameters are correctly added to the parameters so that Drop Columns etc get the right parameters

I think this would be simpler if 1 was a tuner method, like get_starting_parameters ?

… bc_search_parameters

freddyaboulton

@bchen1116 Thank you for your work on this! I left some suggestions for testing improvements. This is looking pretty good though.

docs/source/release_notes.rst

evalml/automl/automl_algorithm/automl_algorithm.py

freddyaboulton · 2022-03-22T18:02:01Z

evalml/automl/automl_search.py

                )
-            if self._sampler_name not in parameters and self._sampler_name is not None:
-                parameters[self._sampler_name] = {
+            if (


Can this be moved to the AutoMLAlgorithm? It's kind of awkward that there parameters are set in AutoMLSearch while the rest are set in the AutoMLAlgorithm.

@freddyaboulton I think this would be a weird move. We use a lot of information that isn't massed to the AutoMLAlgorithm to determine whether we use a sampler and which sampler to use. We would need to pass all of this relevant data to the AutoMLAlgorithm in order to move this logic, and I'm not sure if that's worth it.

I'm on the fence on this:
On one side, I made the decision to move pipeline building into the algorithms and this certainly falls under that category. On the other side, I do understand @bchen1116's concern about bloat in AutoMLAlgorithm. @bchen1116 can you file an issue and use this discussion as context for it?

Yea If the long term plan is to move pipeline building logic to the algorithms then I think the logic for determining whether or not to add a sampler should move to the algorithms. I think there are some unused parameters in the automl algos right now that can be cleaned up too, e.g. number_features. We can do that in a separate issue.

Filed issue here

evalml/tests/automl_tests/test_automl.py

evalml/tuners/tuner.py

evalml/automl/automl_algorithm/iterative_algorithm.py

evalml/tests/automl_tests/test_automl.py

docs/source/user_guide/automl.ipynb

freddyaboulton · 2022-03-22T19:31:42Z

evalml/tests/automl_tests/test_automl_algorithm.py

+    assert aml._tuners.keys() == aml_add_pipelines._tuners.keys()
+    assert aml._tuner_class == aml_add_pipelines._tuner_class
+    aml.next_batch()
+    aml._transform_parameters(None, None)


What does this line do?

Codecov would raise errors if I didn't have calls to the next_batch and _transform_parameters methods. This was to satisfy that

evalml/tests/automl_tests/test_automl.py

jeremyliweishih

Really great work @bchen1116, a big value add in cleaning up the internal API as well as the external parameters API. Appreciate the cleanup in DefaultAlgo as well! Just left some general comments.

evalml/automl/automl_algorithm/automl_algorithm.py

evalml/automl/automl_algorithm/default_algorithm.py

evalml/automl/automl_search.py

jeremyliweishih · 2022-03-23T17:57:14Z

evalml/automl/automl_search.py

                )
-            if self._sampler_name not in parameters and self._sampler_name is not None:
-                parameters[self._sampler_name] = {
+            if (


I'm on the fence on this:
On one side, I made the decision to move pipeline building into the algorithms and this certainly falls under that category. On the other side, I do understand @bchen1116's concern about bloat in AutoMLAlgorithm. @bchen1116 can you file an issue and use this discussion as context for it?

evalml/tests/automl_tests/test_automl.py

… replace with `search_parameters` (#3373)" This reverts commit b442453.

… replace with `search_parameters`" (#3410) * Revert "Remove `pipeline_parameters` and `custom_hyperparameters` and replace with `search_parameters` (#3373)" This reverts commit b442453. * Release notes.

initial impl:

02d717e

bchen1116 self-assigned this Mar 14, 2022

update release notes

85e0513

bchen1116 added 3 commits March 14, 2022 16:11

fix notebook

ddc5bb6

fix docs

5cd9917

add test

8c47ffb

bchen1116 marked this pull request as ready for review March 14, 2022 20:52

bchen1116 requested a review from a team March 14, 2022 20:52

bchen1116 added 3 commits March 15, 2022 10:52

update test

70f8e5c

update impl

e7fcb92

Merge branch 'main' into bc_search_parameters

85b5374

eccabay reviewed Mar 15, 2022

View reviewed changes

docs/source/release_notes.rst Show resolved Hide resolved

docs/source/user_guide/automl.ipynb Outdated Show resolved Hide resolved

docs/source/user_guide/automl.ipynb Outdated Show resolved Hide resolved

bchen1116 added 2 commits March 15, 2022 14:23

update docs

f21f012

Merge branch 'main' into bc_search_parameters

2d97b9f

freddyaboulton reviewed Mar 15, 2022

View reviewed changes

bchen1116 added 9 commits March 15, 2022 18:24

update implementation to use tuner to suggest

ad0e0fb

Merge branch 'bc_search_parameters' of github.com:alteryx/evalml into…

e770926

… bc_search_parameters

make changes to how automl algo handles pipelines

828a8f4

update test

e1dda1f

Merge branch 'main' into bc_search_parameters

eaa0663

rerun test

ce0db41

update docstring

844b042

fix import

cb26e74

address codecov

6e4a4f7

bchen1116 requested review from eccabay and freddyaboulton March 16, 2022 22:00

bchen1116 added 3 commits March 17, 2022 13:13

Merge branch 'main' into bc_search_parameters

204c0d8

Merge branch 'main' into bc_search_parameters

74094c0

Merge branch 'main' into bc_search_parameters

a760c8a

bchen1116 added 6 commits March 22, 2022 10:54

pipeline parameters

4050ed8

lint

49432a2

testing

d68c940

test commit hook

26860c6

Merge branch 'main' into bc_search_parameters

9b2d682

Merge branch 'main' into bc_search_parameters

e953b06

freddyaboulton approved these changes Mar 22, 2022

View reviewed changes

Merge branch 'main' of github.com:alteryx/evalml

a7545cf

bchen1116 mentioned this pull request Mar 22, 2022

Remove algorithm logic out of AutoML tests #2868

Open

bchen1116 added 7 commits March 22, 2022 17:15

update with comments

be7b26a

lint

e557fdf

update tuner

fc1d36a

update

a34d027

Merge branch 'main' of github.com:alteryx/evalml

3a39f0e

merge

5703bbe

fix docs

e75fec9

jeremyliweishih approved these changes Mar 23, 2022

View reviewed changes

bchen1116 mentioned this pull request Mar 23, 2022

Move pipeline parameter logic into AutoMLAlgorithm #3400

Open

bchen1116 added 5 commits March 23, 2022 15:24

update with comments

e053263

lint

b52d9d1

fix test

73952c8

rerun test

2993bcb

Merge branch 'main' into bc_search_parameters

2847634

bchen1116 merged commit b442453 into main Mar 24, 2022

chukarsten mentioned this pull request Mar 25, 2022

Release v0.48.0 #3408

Merged

chukarsten added a commit that referenced this pull request Mar 28, 2022

Revert "Remove pipeline_parameters and custom_hyperparameters and…

c677752

… replace with `search_parameters` (#3373)" This reverts commit b442453.

chukarsten mentioned this pull request Mar 28, 2022

Revert "Remove pipeline_parameters and custom_hyperparameters and replace with search_parameters" #3410

Merged

jeremyliweishih mentioned this pull request Apr 5, 2022

Add test to ensure positive_label passed properly in AMLSearch #3326

Merged

chukarsten mentioned this pull request Apr 12, 2022

Release v.0.50.0 #3461

Merged

Conversation

bchen1116 commented Mar 14, 2022

Uh oh!

codecov bot commented Mar 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eccabay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freddyaboulton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bchen1116 Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeremyliweishih left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Mar 14, 2022 •

edited

Loading

bchen1116 Mar 22, 2022 •

edited

Loading