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

Defer importing devtools in sitecustomize.py. #49

Closed
wants to merge 4 commits into from

Conversation

kalekundert
Copy link

It's convenient to be able to use debug() as if it were a builtin function, but importing devtools directly in sitecustomize.py is a pretty significant performance hit for every invocation of python that doesn't use debug(). Specifically, this import takes 100-200 ms, which is a noticeable delay for interactive sessions and short-running scripts:

$ python -X importtime -c 'import devtools'

While it would probably be possible to make devtools faster to import, the best solution is simply to avoid importing devtools unless it will be used. It took me a while to figure out how to do this, but in the end the code is pretty clear and succinct, and I think it will be useful to others:

# sitecustomize.py
class lazy_debug:
    @property
    def __call__(self):
        from devtools import debug
        return debug

__builtins__['debug'] = lazy_debug()

This PR updates the README file to include the above snippet.

It's convenient to be able to use `debug()` as if it were a builtin
function, but importing `devtools` directly in `sitecustomize.py` is a
pretty significant performance hit for every invocation of python that
doesn't use `debug()`.  Specifically, this import takes 100-200 ms,
which is a noticeable delay for interactive sessions and short-running
scripts:

    $ python -X importtime -c 'import devtools'

While it would probably be possible to make `devtools` faster to import,
the best solution is simply to avoid importing `devtools` unless it will
be used.  It took me a while to figure out how to do this, but in the
end the code is pretty clear and succinct, and I think it will be useful
to others.
@codecov
Copy link

codecov bot commented Apr 16, 2020

Codecov Report

Merging #49 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master       #49   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines          499       499           
  Branches        79        79           
=========================================
  Hits           499       499           

@samuelcolvin
Copy link
Owner

I hadn't noticed this, thanks so much for pointing it out.

The main problem with this approach is that other tools attached to debug like debug.timer don't work with this approach. See here.

I know 99.9% of debug usage is just debug(), indeed I had to look up these other tools as even I had forgotten exactly where they were. But I still think it's confusing to disable them like this.

There's also an example in the docs that isn't updated.

The other problem is that debug import could be speeded up a lot, see pydantic/pydantic#1127 for a discussion of something similar.

In summary I think we should:

  • remove all import/exports from devtools/__init__.py
  • see how much we can speed up from devtools import debug by deferring other package imports.
  • add devtools/lazy.py module with only contains a thin wrapper around the standard debug instance, so sitecustomize.py can just contain
from devtools.lazy import lazy_debug
__builtins__['debug'] = lazy_debug

@samuelcolvin
Copy link
Owner

See #50 - I think if we can improve the import time from 100-200ms to 10-20ms, perhaps we don't need a lazy import workaround?

@kalekundert
Copy link
Author

That all sounds really good, especially the idea of adding a devtools.lazy module. I'd personally want to keep using a lazy loader even if import time got down to 10-20 ms, just because I feel like everything in sitecustomize.py should be as fast as possible, but you're right that the difference would be imperceptible at that point.

I didn't know about debug.timer() and friends, although they seem useful. I added a __getattr__() method to the lazy wrapper to properly expose those methods as well.

@samuelcolvin
Copy link
Owner

okay, happy to accept this PR, but could you update the duplicate code in the docs docs/examples/sitecustomize.py?

@kalekundert
Copy link
Author

Sorry, you mentioned that earlier and I didn't notice. Should be up to date now.

@samuelcolvin
Copy link
Owner

I think this is solved by #50, I'll make a release soon.

@alexmojaki
Copy link
Contributor

FWIW here's how I solve a similar problem in birdseye: https://github.com/alexmojaki/birdseye/blob/master/birdseye/__init__.py

It's a lazy object similar to this PR but it's part of the library itself, not just a sitecustomize recipe. It's worked great for me.

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

3 participants