Timeout parameter to limit time (in seconds) of the search process (fmin) #533

vladkar · 2019-09-03T17:27:01Z

max_time parameter was added to fmin:

stops searching process when allowed time is passed, works similar to max_evals parameter, but limits time of search instead of number of trials
can work together with max_evals
max_time=60 means that search process will be stopped after 60 seconds since its start with respect to last trial (if trial requires 10 seconds and it was started at second 59, then search process takes 69 second)
unit tests were prepared to check new parameter
progress bar was not changed (it works based on number of trials)

Update 11 Dec 2019:

max_time parameter was renamed to timeout to keep consistency with recent update
timeout parameter was added to fmin function for spark implementation as well (to keep configuration of timeout and max_evals in one place)

marctorsoc

Some initial comments. You should also merge with master as now there are conflicts

marctorsoc · 2019-12-01T10:19:23Z

hyperopt/fmin.py

@@ -9,6 +9,7 @@
 import os
 import sys
 import time
+import timeit


given you only use default_timer, what do you think about changing this to from timeit import default_timer as timer ?

marctorsoc · 2019-12-01T10:22:35Z

hyperopt/fmin.py

                    qlen = get_queue_len()
-                    while qlen < self.max_queue_len and n_queued < N:
+                    while qlen < self.max_queue_len and n_queued < N and (timeit.default_timer() - self.start_time) < self.max_time:


are u sure u need to check the time twice? can line 205 take long to run?

One is not required, condition was optimized

marctorsoc · 2019-12-01T10:27:20Z

hyperopt/tests/test_fmin.py

@@ -212,3 +214,36 @@ def test_generate_trials_to_calculate(self):
                    )
        assert best['x'] == 0.0
        assert best['y'] == 0.0
+
+
+def test_max_time():


this probably can be written in a for loop. Ideally with pytest it would be with pytest.mark.parametrize but hyperopt doesn't use pytest yet

True, it could be a cycle, but I intentionally keep it like this to make test readable and fast (taking into account that we literally need to wait to test timeout) as possible.

marctorsoc · 2019-12-01T10:34:02Z

see #311. It seems to me the aim is the same, so it'd be nice to work together and close 2 PRs in 1 effort, if possible

Refactor timeout validation (logic is moved to base class)

# Conflicts (all resolved by theirs): # hyperopt/base.py # hyperopt/fmin.py # hyperopt/tests/test_fmin.py

vladkar · 2019-12-11T13:11:16Z

Pull request was updated:

max_time parameter was renamed to timeout to keep consistency with recent update
timeout parameter was added to fmin function (to keep configuration of timeout and max_evals in one place)

There are some failed tests at spark functionality, I am still working on it.

marctorsoc · 2019-12-11T14:10:15Z

hyperopt/fmin.py

@@ -234,7 +239,8 @@ def get_n_unfinished():
        ) as progress_ctx:

            all_trials_complete = False
-            while n_queued < N or (block_until_done and not all_trials_complete):
+            while (n_queued < N or (block_until_done and not all_trials_complete)) and \
+                    (self.timeout is None or (timer() - self.start_time) < self.timeout):


this can be timer() - self.start_time < (self.timeout or float('inf')). Also, please use black to format code

It can, but version I used (a) subjectively more readable, (b) objectively faster

Black was applied

marctorsoc · 2019-12-11T14:12:20Z

hyperopt/base.py

+    if timeout is not None and (
+            not isinstance(timeout, numbers.Number)
+            or timeout <= 0
+            or isinstance(timeout, bool)


I think this is redundant. If it's not a number, you don't need to check if it's a bool

also, if the docstring says int, then I think better to check here is int, rather than any number

This is not my code segment, I used check from spark implementation, but since I use the same parameter I just moved this check to separate method to base file. To not break something by accident I did not change the logic of this validation.

if you can reuse code from somwhere else, do it. Do not duplicate

Either the docstring or check need to be changed, to match. The check allows for floats and has a check for bool that seems useless to me, while the docstring states int. I would change this code as I don't think we need float precision for a timeout in seconds

It was reused, not duplicated. As I mention, this check was just moved to base.

I think this should have been discussed when this check was introduced to spark implementation. Again, I prefer to reuse it than make (almost) the same new check. And I do not want to influence existing logic, where this check is already used.

marctorsoc · 2019-12-11T14:15:21Z

hyperopt/fmin.py

@@ -371,6 +378,10 @@ def fmin(
    max_evals : int
        Allow up to this many function evaluations before returning.

+    timeout : int


please use style from other params :

timeout: None or int. Limits ... If None, then the search process has no time constraint.

marctorsoc · 2019-12-11T14:16:07Z

hyperopt/tests/test_fmin.py

+
+def test_timeout():
+    fn = lambda x: [time.sleep(1), x][1]
+    space = hp.choice('x', range(20))


please use black

vladkar · 2019-12-11T14:26:19Z

Ok, stuff was resolved

vladkar · 2019-12-11T15:14:11Z

I guess there is issue with test 'test_task_maxFailures_warning', its result is inconsistent (sometimes it is ok, sometimes it is failed) even on pure master branch. See log as an example: build was ok, then it fails after cosmetic commit.

marctorsoc · 2019-12-11T18:52:07Z

I don't know about that test. Could you please make it deterministic?

marctorsoc · 2019-12-11T21:19:31Z

I don't know about that test. Could you please make it deterministic?

forget about it, seems to work. Ok, will review when I can and approve. @maxpumperla can you take a look at this?

maxpumperla · 2019-12-12T07:39:45Z

@marctorrellas we need to look at unrelated tests separately

@vladkar great work, I have nothing to add. moving that check to base is fair, you don't have to clean up other PRs here. 👍

anything else you wanted to change or are we good to go from your side?

vladkar · 2019-12-12T11:03:10Z

@marctorrellas @maxpumperla thank you for your feedback, that is it from my side for this PR.

marctorsoc · 2019-12-12T14:08:42Z

@vladkar good job, thanks for your contribution

marctorsoc · 2019-12-12T14:09:51Z

@maxpumperla when is the next release expected? this and the functionality about verbosity are cool things I would like see soon :) count on me if you need a hand

maxpumperla · 2019-12-12T19:59:56Z

@marctorrellas I want to fix that (apparently still around) BSON issue (#547) and look into lower/upper bounds for randint (#536) as a minimum here. got time to look at any one of these?

vlad added 3 commits September 3, 2019 19:24

Add max_time parameter and its considering in cycle

03b21e1

Add test for max_time parameter

f00c409

Merge branch 'master' into max-time-param

ec07715

marctorsoc requested changes Dec 1, 2019

View reviewed changes

marctorsoc mentioned this pull request Dec 1, 2019

Added timing constraint #311

Closed

Karbovskii Vladislav added 8 commits December 10, 2019 18:11

Add simple timeout param

65e9937

Add test for timeout param

822e237

Add timeout param to spark fmin

5be628a

Refactor timeout validation (logic is moved to base class)

Timer import refactor

daa7ba5

Merge branch 'timeout-param' into max-time-param

d46e39d

# Conflicts (all resolved by theirs): # hyperopt/base.py # hyperopt/fmin.py # hyperopt/tests/test_fmin.py

Fix lost import

00a846f

Merge branch 'timeout-param' into max-time-param

c73e52c

Refactor timer import

ca15fd4

vladkar changed the title ~~max_time parameter to limit time (in seconds) of the search process (fmin)~~ Timeout parameter to limit time (in seconds) of the search process (fmin) Dec 11, 2019

Karbovskii Vladislav added 2 commits December 11, 2019 17:16

Correct spark timeout declaration via fmin

7b01eaf

Add test for spark timeout declaration via fmin

1717f5f

marctorsoc requested changes Dec 11, 2019

View reviewed changes

Karbovskii Vladislav added 2 commits December 11, 2019 17:49

Apply black to changed files

d3e98f7

Update param description style

53ced30

Karbovskii Vladislav added 2 commits December 11, 2019 18:54

Add invalid timeout test

497767d

Change string format style

3b47284

maxpumperla merged commit 11632b2 into hyperopt:master Dec 12, 2019

marctorsoc mentioned this pull request Dec 16, 2019

Make tqdm progress optional #583

Merged

marctorsoc mentioned this pull request Dec 30, 2019

Search number definition: max_evals or confidence #591

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timeout parameter to limit time (in seconds) of the search process (fmin) #533

Timeout parameter to limit time (in seconds) of the search process (fmin) #533

vladkar commented Sep 3, 2019 •

edited

marctorsoc left a comment

marctorsoc Dec 1, 2019

vladkar Dec 11, 2019

marctorsoc Dec 1, 2019

vladkar Dec 11, 2019 •

edited

marctorsoc Dec 1, 2019

vladkar Dec 11, 2019

marctorsoc commented Dec 1, 2019

vladkar commented Dec 11, 2019 •

edited

marctorsoc Dec 11, 2019

vladkar Dec 11, 2019

vladkar Dec 11, 2019

marctorsoc Dec 11, 2019

marctorsoc Dec 11, 2019

vladkar Dec 11, 2019

marctorsoc Dec 11, 2019

vladkar Dec 11, 2019 •

edited

marctorsoc Dec 11, 2019

vladkar Dec 11, 2019

marctorsoc Dec 11, 2019

vladkar Dec 11, 2019

vladkar commented Dec 11, 2019

vladkar commented Dec 11, 2019 •

edited

marctorsoc commented Dec 11, 2019

marctorsoc commented Dec 11, 2019

maxpumperla commented Dec 12, 2019

vladkar commented Dec 12, 2019 •

edited

marctorsoc commented Dec 12, 2019

marctorsoc commented Dec 12, 2019

maxpumperla commented Dec 12, 2019

Timeout parameter to limit time (in seconds) of the search process (fmin) #533

Timeout parameter to limit time (in seconds) of the search process (fmin) #533

Conversation

vladkar commented Sep 3, 2019 • edited

marctorsoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladkar Dec 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marctorsoc commented Dec 1, 2019

vladkar commented Dec 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladkar Dec 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladkar commented Dec 11, 2019

vladkar commented Dec 11, 2019 • edited

marctorsoc commented Dec 11, 2019

marctorsoc commented Dec 11, 2019

maxpumperla commented Dec 12, 2019

vladkar commented Dec 12, 2019 • edited

marctorsoc commented Dec 12, 2019

marctorsoc commented Dec 12, 2019

maxpumperla commented Dec 12, 2019

vladkar commented Sep 3, 2019 •

edited

vladkar Dec 11, 2019 •

edited

vladkar commented Dec 11, 2019 •

edited

vladkar Dec 11, 2019 •

edited

vladkar commented Dec 11, 2019 •

edited

vladkar commented Dec 12, 2019 •

edited