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

Added support for unlim pipelines with max_time limit #70

Merged
merged 24 commits into from Nov 1, 2019

Conversation

christopherbunn
Copy link
Contributor

@christopherbunn christopherbunn commented Sep 13, 2019

Added support for the construction of unlimited pipelines, as long as a time limit is specified. If no limits are specified, an error is thrown. (#14)

Copy link
Contributor

@jeremyliweishih jeremyliweishih left a comment

Looking good, just some comments

evalml/models/auto_base.py Outdated Show resolved Hide resolved
evalml/models/auto_base.py Outdated Show resolved Hide resolved
evalml/models/auto_classifier.py Show resolved Hide resolved
Copy link
Contributor

@jeremyliweishih jeremyliweishih left a comment

Looks good. Just one last comment and @kmax12 or @rwedge can do the final run through after merging in master.

evalml/models/auto_base.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 8, 2019

Codecov Report

Merging #70 into master will increase coverage by 0.22%.
The diff coverage is 96.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   95.52%   95.75%   +0.22%     
==========================================
  Files          57       58       +1     
  Lines        1498     1531      +33     
==========================================
+ Hits         1431     1466      +35     
+ Misses         67       65       -2
Impacted Files Coverage Δ
evalml/models/auto_regressor.py 90.9% <ø> (ø) ⬆️
evalml/models/auto_classifier.py 100% <ø> (ø) ⬆️
evalml/tests/automl_tests/test_autoclassifier.py 100% <100%> (ø) ⬆️
evalml/tests/test_autobase.py 100% <100%> (ø)
evalml/models/auto_base.py 93.47% <92%> (+1.31%) ⬆️

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 e092fc0...7dbc123. Read the comment docs.

jeremyliweishih
jeremyliweishih previously requested changes Oct 11, 2019
Copy link
Contributor

@jeremyliweishih jeremyliweishih left a comment

Progress bar needs some changes and I think something is up with the PR with the merged in changes!

start = time.time()
elapsed = 0
last_time = start
with tqdm(total=self.max_time, disable=not self.verbose, file=stdout) as pbar:
Copy link
Contributor

@jeremyliweishih jeremyliweishih Oct 11, 2019

Choose a reason for hiding this comment

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

Currently, the progress bar may be misleading since if training starts at 99% it could hang depending on how long training would take to finish. Maybe we can remove the progress bar and only show elapsed time using bar_format in tqdm or somehow let the bar reflect time elapsed and not dependent on training iterations.

evalml/models/auto_base.py Outdated Show resolved Hide resolved
evalml/models/auto_base.py Outdated Show resolved Hide resolved
evalml/models/auto_classifier.py Show resolved Hide resolved
evalml/models/auto_base.py Outdated Show resolved Hide resolved
@christopherbunn christopherbunn requested a review from kmax12 Oct 28, 2019
@kmax12
Copy link
Contributor

kmax12 commented Oct 29, 2019

@christopherbunn can we document this new functionality? i think the best place would be in this section https://evalml.featurelabs.com/en/latest/automl/pipeline_search.html#Limiting-Search-Time

evalml/models/auto_base.py Outdated Show resolved Hide resolved
evalml/models/auto_base.py Outdated Show resolved Hide resolved
evalml/models/auto_base.py Show resolved Hide resolved
Copy link
Contributor

@kmax12 kmax12 left a comment

almost there! two more comments

start = time.time()
elapsed = 0
pbar = tqdm(total=self.max_time, disable=not self.verbose, file=stdout, bar_format='{desc} | Elapsed:{elapsed}')
while elapsed <= self.max_time:
Copy link
Contributor

@kmax12 kmax12 Oct 31, 2019

Choose a reason for hiding this comment

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

can we just make this while time.time() - start <= self.max_time: and remove the elapsed variable?

evalml/models/auto_base.py Show resolved Hide resolved
kmax12
kmax12 approved these changes Oct 31, 2019
Copy link
Contributor

@kmax12 kmax12 left a comment

LGTM

@kmax12 kmax12 dismissed jeremyliweishih’s stale review Oct 31, 2019

all comments addressed

@christopherbunn christopherbunn merged commit 957ea7f into master Nov 1, 2019
@christopherbunn christopherbunn deleted the unlim-pipelines branch Nov 1, 2019
@angela97lin
Copy link
Contributor

angela97lin commented Nov 1, 2019

@christopherbunn if it's not too much of a hassle, could you move the changelog line to "Future Releases" instead of under v.0.5.0?

@christopherbunn christopherbunn restored the unlim-pipelines branch Nov 1, 2019
@christopherbunn
Copy link
Contributor Author

christopherbunn commented Nov 1, 2019

@angela97lin Sure

angela97lin pushed a commit that referenced this pull request Nov 1, 2019
* Added support for unlim pipelines with max_time limit

* Fixed lint errors

* Increased number of test_binary_auto pipelines to 5

* Fixed max_pipelines=None behavior and removed extraneous comment

* Revered some Autoclassifier tests to use max_pipelines=5

* Changed the format of the progress logs for max_time

* Changed to new pbar format and modified error msg

* Updated notebook example to include search limit

* Updated limit handling to allow for no time parameters

* Fixed lint errors

* Updated changelog

* Closed pbear on early termination and removed new_line

* Status bar changes

* Fixed lint error

* Updated test and removed elasped variable

* Fixed position in changelog

* Removed duplicate line
@angela97lin angela97lin mentioned this pull request Nov 15, 2019
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.

None yet

4 participants