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

timeit called from within Python should allow autoranging #50671

Closed
scottdaniels mannequin opened this issue Jul 5, 2009 · 30 comments
Closed

timeit called from within Python should allow autoranging #50671

scottdaniels mannequin opened this issue Jul 5, 2009 · 30 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@scottdaniels
Copy link
Mannequin

scottdaniels mannequin commented Jul 5, 2009

BPO 6422
Nosy @rhettinger, @amauryfa, @ncoghlan, @pitrou, @vstinner, @rbtcollins, @merwok, @stevendaprano, @asvetlov, @csabella
Files
  • timeit.patch: Patch including changes to timeit, test, and doc
  • timeit-autorange.patch: minimal proof-of-concept patch to add autorange method to Timer
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/stevendaprano'
    closed_at = <Date 2019-03-28.09:50:03.233>
    created_at = <Date 2009-07-05.15:15:10.511>
    labels = ['type-feature', 'library']
    title = 'timeit called from within Python should allow autoranging'
    updated_at = <Date 2019-03-28.09:50:03.232>
    user = 'https://bugs.python.org/scottdaniels'

    bugs.python.org fields:

    activity = <Date 2019-03-28.09:50:03.232>
    actor = 'cheryl.sabella'
    assignee = 'steven.daprano'
    closed = True
    closed_date = <Date 2019-03-28.09:50:03.233>
    closer = 'cheryl.sabella'
    components = ['Library (Lib)']
    creation = <Date 2009-07-05.15:15:10.511>
    creator = 'scott_daniels'
    dependencies = []
    files = ['14472', '42816']
    hgrepos = []
    issue_num = 6422
    keywords = ['patch']
    message_count = 30.0
    messages = ['90157', '90245', '90264', '90275', '122955', '123711', '164027', '164070', '164071', '164216', '164217', '168796', '238352', '238354', '238879', '265319', '265322', '265324', '265326', '265360', '265418', '265442', '272140', '272142', '272676', '272704', '338958', '339011', '339024', '339026']
    nosy_count = 13.0
    nosy_names = ['rhettinger', 'scott_daniels', 'amaury.forgeotdarc', 'ncoghlan', 'pitrou', 'vstinner', 'rbcollins', 'eric.araujo', 'steven.daprano', 'asvetlov', 'tshepang', 'python-dev', 'cheryl.sabella']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue6422'
    versions = ['Python 3.6']

    @scottdaniels
    Copy link
    Mannequin Author

    scottdaniels mannequin commented Jul 5, 2009

    timeit.main has a _very_ handy autoranging facility to pick an
    appropriate number of repetitions when not specified. The autoranging
    code should be lifted to a method on Timer instances (so non-main code
    can use it). If number is specified as 0 or None, I would like to use
    the results of that autoranging code in Timer.repeat and Timer.timeit.

    Patch to follow.

    @scottdaniels scottdaniels mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Jul 5, 2009
    @scottdaniels
    Copy link
    Mannequin Author

    scottdaniels mannequin commented Jul 7, 2009

    I've got the code "working" on trunk2 for my tests.
    Should I port to py3K before checking in, and give diffs from there, or
    what?

    @amauryfa
    Copy link
    Member

    amauryfa commented Jul 8, 2009

    You can still upload available patches to this tracker.

    @rhettinger
    Copy link
    Contributor

    I would like to look at this in context of all the other proposed build-
    outs to timeit.

    @rhettinger rhettinger self-assigned this Jul 8, 2009
    @rhettinger
    Copy link
    Contributor

    This does not conflict with the other proposed changes to timeit and it is in-line with Guido's desire that to expose useful parts currently buried in the command-line logic.

    Amaury, you've shown an interest. Would you like to apply it?

    @rhettinger rhettinger assigned amauryfa and unassigned rhettinger Nov 30, 2010
    @pitrou
    Copy link
    Member

    pitrou commented Dec 9, 2010

    Not sure why you chose 0.11 here. It should probably be 0.2 as in the command-line code.
    As for applying the patch, this can't be done before 3.2 is released.

    @rhettinger
    Copy link
    Contributor

    Looking at this again after more time has passes, I still think exposing autoranging is a good idea but I don't like the patch as it stands. It "infects" the API in a number of places and makes the overall module harder to use and learn.

    Ideally, there should be a cleaner interface, or more limited API change, or a separate high level function that can autorange existing functions without changing their API.

    Anyone care to propose a cleaner API?

    @ncoghlan
    Copy link
    Contributor

    In bpo-5442, I proposed leaving the architecture of the module alone, and simply exposing the main module functionality as a high level helper function:

    def measure(stmt="pass", setup="pass", timer=default_timer,
                repeat=default_repeat, number=default_number,
                verbosity=0, precision=3)

    The return value would simply be a (number, results) 2-tuple with the number of iterations per test (which may have been calculated automatically), and then a list of the results. To get "timeit" style behavior, simply set "repeat=1".

    @ncoghlan
    Copy link
    Contributor

    Oops, that link reference should have been to bpo-5441.

    @vstinner
    Copy link
    Member

    Hi, I wrote recently a similar function because timeit is not reliable by default. Results look random and require to run the same benchmark 3 times or more on the command line.

    https://bitbucket.org/haypo/misc/src/tip/python/benchmark.py

    By default, the benchmark takes at least 5 measures, one measure should be greater than 100 ms, and the benchmark should not be longer than 1 second. I chose these parameters to get reliable results on microbenchmarks like "abc".encode("utf-8").

    The calibration function uses also the precision of the timer. The user may define a minimum time (of one measure) smaller than the timer precision, so the calibration function tries to solve such issue. The calibration computes the number of loops and the number of repetitions.

    Look at BenchmarkRunner.calibrate_timer() and BenchmarkRunner.run_benchmark().
    https://bitbucket.org/haypo/misc/src/bfacfb9a1224/python/benchmark.py#cl-362

    @vstinner
    Copy link
    Member

    The calibration function uses also the precision of the timer.

    Oh, I forgot to mention that it computes the precision in Python, it doesn't read the precision announced by the OS or the precision of the C structure.
    https://bitbucket.org/haypo/misc/src/bfacfb9a1224/python/benchmark.py#cl-66

    @pitrou
    Copy link
    Member

    pitrou commented Aug 21, 2012

    In bpo-5442, I proposed leaving the architecture of the module alone, and
    simply exposing the main module functionality as a high level helper
    function:

    Agreed with Nick's approach above.

    Victor, if you want to improve timeit's reliability, please open a separate issue.

    @rbtcollins
    Copy link
    Member

    I'm confused by the feedback on the patch. It adds a single new function, doesn't alter the public interface for any existing functions, and seems fit for purpose. Could someone help me understand how its deficient?

    @rbtcollins
    Copy link
    Member

    Filed bpo-23693 for the accuracy thing.

    @ncoghlan
    Copy link
    Contributor

    The current patch moves print operations inside timeit() and repeat(), instead of leaving the main() function as the only one with side effects.

    My counter-proposal was to instead extract the current main functionality out into a side-effect free public API of its own, and change the existing main function to call that new API and print the results.

    @stevendaprano
    Copy link
    Member

    This issue seems to have lost momentum, I'd like to revive it by proposing a slightly different interface for the autorange function.

    Attached is a proof-of-concept patch. I've moved the code which determines the number of loops out of the main function into a new Timer method, autorange. The main difference between my approach and Scott's is that my autorange method doesn't do any printing directly, it takes an optional callback function. This lets the caller take responsibility for all output.

    If this approach is acceptable, I hope to:

    • (in 3.6) add tests and docs for the new method;

    • (in 3.6 if time permits, otherwise 3.7) modify the timeit and repeat methods and functions so that they can optionally call autorange(), e.g. if the caller passes 0 as the number.

    I'm not sure that there's a good reason to add a top-level autorange() function to match the timeit() and repeat() functions. Especially not once they gain the ability to autorange themselves.

    I think my approach will be compatible with cleaning up and refactoring the main() function. At the moment, main() is a mess IMO, it handles argument processing, autoranging, units of time, and unreliable timing detection all from one function.

    @pitrou
    Copy link
    Member

    pitrou commented May 11, 2016

    I would suggest making the 0.2 tunable as an optional argument. Different applications (benchmarks) may want different duration / precision tradeoffs.
    I also notice the repeat functionality isn't included in the patch, is there a reason?

    @stevendaprano
    Copy link
    Member

    I would suggest making the 0.2 tunable as an optional argument.

    Sounds like a good idea to me.

    I also notice the repeat functionality isn't included in the patch, is there a reason?

    I don't understand which repeat functionality you're referring to.

    @pitrou
    Copy link
    Member

    pitrou commented May 11, 2016

    I don't understand which repeat functionality you're referring to.

    https://docs.python.org/3/library/timeit.html#timeit.Timer.repeat

    (or, similarly, what timeit's __main__ does: report the minimum of all N runs)

    @ncoghlan
    Copy link
    Contributor

    The embedded side-effects were my main concern with Scott's original patch, so Steven's callback-based approach strikes me as a definite improvement. However, the awkwardness of the revised calling code in main does make me wonder whether or not this might be better implemented as a generator rather than as a function accepting a callback:

    try:
        results = list(t.autorange())
    except:
        t.print_exc()
        return 1
    if verbose:
        for number, time_taken in results:
            msg = "{} loops -> {:.{}g} secs"
            print(msg.format(number, time_taken, precision))
    

    (Originally I had the "if verbose: print" embedded in a direct loop over t.autorange(), but writing it that way made it immediately clear that the scope of the exception handler was too broad, so I changed it to extract all the results and only then print them)

    @stevendaprano
    Copy link
    Member

    On Thu, May 12, 2016 at 04:49:59AM +0000, Nick Coghlan wrote:

    The embedded side-effects were my main concern with Scott's original
    patch, so Steven's callback-based approach strikes me as a definite
    improvement. However, the awkwardness of the revised calling code in
    main does make me wonder whether or not this might be better
    implemented as a generator rather than as a function accepting a
    callback:

    I thought about a generator too, but then I thought about the *non*
    verbose case, where you don't care about the intermediate results, only
    the final (number, time_taken) pair.

    # function with callback:
    number, time_taken = t.autorange()

    # generator
    number, time_taken = list(t.autorange())[-1]

    Which hints that your code snippet is buggy, or at least incomplete:

    try:
        results = list(t.autorange())
    except:
        t.print_exc()
        return 1
    if verbose:
        for number, time_taken in results:
            msg = "{} loops -\> {:.{}g} secs"
            print(msg.format(number, time_taken, precision))
    

    If verbose is False, you never set number and time_taken. So you need an
    else clause:

    else:
        number, time_taken = results[-1]
    

    @ncoghlan
    Copy link
    Contributor

    Good point - given that, +1 from me for the callback based version, especially since exception chaining will still disambiguate failures in the callback from other errors.

    @stevendaprano
    Copy link
    Member

    Nick gave a +1 to my auto-range patch with callback on 2016-05-13, and there's been no negative feedback since. Should I go ahead and check it in for 3.6?

    @rhettinger
    Copy link
    Contributor

    I think the patch is good to go.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 14, 2016

    New changeset 424eb46f7f3a by Steven D'Aprano in branch 'default':
    bpo-6422 add autorange method to timeit.Timer
    https://hg.python.org/cpython/rev/424eb46f7f3a

    @stevendaprano
    Copy link
    Member

    Still to do (coming soon):

    • make the 0.2s time configurable;
    • have timeit and repeat methods (and functions) fall back
      on autorange if the number is set to 0 or None.

    @stevendaprano stevendaprano self-assigned this Aug 15, 2016
    @csabella
    Copy link
    Contributor

    Hello Steven,

    Were you working on the additional functionality that you mentioned in msg272704 or would that be open for someone else to do? Thanks!

    @stevendaprano
    Copy link
    Member

    Were you working on the additional functionality that you mentioned in
    msg272704 or would that be open for someone else to do? Thanks!

    Please consider it open. I don't expect it to be difficult, it's just
    finding the Round Tuits. Perhaps an easy first issue for someone?

    @csabella
    Copy link
    Contributor

    Steven,

    Thank you. Yes, I was thinking the same thing. But it might be better at this point for that change to have its own ticket, so I'll open a new issue for it.

    @csabella
    Copy link
    Contributor

    The new ticket is bpo-36461.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants