Remove `IPython` dependency from `fire.Fire` or improve load times some other way

[ubershmekel]

import IPython takes about 0.5 seconds on my machine to import and is the longest part of running simple fire scripts at the moment. Launching a python script and printing something takes 0.15 seconds so the total with import fire becomes about 0.65 seconds.

It's only used in _EmbedIPython and inspectutils.py so maybe it can be optional for command line apps and the inspection part ported over (or placed in a different smaller package)? Would such a pull request be useful?

[dbieber]

I agree that this is an important thing to improve, and it deserves some discussion first.

As you point out, IPython only used in two places:

• inspectutils.py only uses a single function from IPython (IPython.core.oinspect.Inspector().info), so maybe we could just strip that one function out of IPython or write an equivalent function from scratch. I would want permission from the IPython folks before doing the former.
• interact.py uses IPython for the --interactive flag, but this should really be optional. You'll notice the unused _EmbedCode function in interact.py. (https://github.com/google/python-fire/blob/master/fire/interact.py#L89) The reason that's there is to eventually enable Fire to be used without IPython as a required dependency.

The first course of action I'm leaning toward is to lazily import IPython in the two places it's used. If we do this then the lazy import (and the corresponding 0.5 second delay) will only trigger 1) when --interactive mode is used, and 2) when generating usage strings. We'll need to add the appropriate annotations to keep the linter happy since we'll be violating some PEP by lazy importing.

The second related thing I think we should do is make IPython an optional dependency for interact.py. If the user doesn't have IPython, then we should fall back to using code (a builtin module) instead. For this, we'll have to figure out a good way to make sure tests cover both of these situations. And I'm sure we'll be making the linter even more unhappy.

Thirdly, it would be good to remove inspectutils.py's dependency on IPython altogether. As noted above, we have two options for doing this (rewrite the info function or get permission from IPython to just take it).

Once all this is done, we could remove IPython from setup.py altogether or list it as an optional suggested dependency, if such a thing exists.

Thoughts?

[fruch]

My vote is for optional usage of ipython as a subpackage, like pip install fire[ipython] only for the interactive mode to work. The default should be lighter. Most of my users won't ever use IPython.

As for the borrowing code from other IPython, that's also a good idea (maybe later it can become a share inspection of package)

[tmr232]

Adding IPython devs to ask for their take on it .
@minrk @takluyver

[minrk]

We've made some changes in IPython recently that have slowed down import. We are looking into ways that we can address that. I think it's definitely a good idea to only import IPython if it's actually going to be invoked, rather than unconditionally. I think lazy imports are generally a good idea if you have a fair amount of optional functionality.

As for the dependency, if a minority of users actually use it, making it an optional dependency (or no dependency at all) makes sense to me. You can always have the interactive mode do something like:

try:
    import IPython
except ImportError:
    raise ImportError("IPython is required for interactive mode, `pip install ipython` to get it...")

[dbieber]

Thanks for chiming in.

• Lazy importing resolves the speed issue in the common case.
• try/except (and falling back to the builtin 'code' module, rather than raising as error) makes the IPython dependency optional for interactive mode.

The one piece that remains is our use of IPython.core.oinspect.Inspector().info in inspectutils.py. It seems excessive to keep all of IPython as a dependency for this one function. As noted above, our options here are to rewrite the parts of the info function we need from scratch, or to copy it from IPython, provided we have the maintainers' blessings to do so. What are your thoughts on this, @minrk?

[minrk]

If copying it out is what you want, that's totally fine under IPython's BSD license. I think it's a nontrivial bit of code, though.

[dbieber]

Part 1 (lazy imports of IPython) is done w/ 3db47f8.
This should solve the initial reported issue of a 0.5 second delay in the common case.

[dbieber]

https://github.com/google/python-fire/tree/z2017-05-21-ipython-optional (e.g. 3950004) would complete this ticket, fully making IPython an optional dependency.

• Still needs tests for both the IPython and non-IPython cases.
• The new _InfoBackup function is missing a number of edge cases.

[dbieber]

Still needs tests for both the IPython and non-IPython cases.

Done. (on the z2017-05-21-ipython-optional branch)

The new _InfoBackup function is missing a number of edge cases.

Since we're moving this info to only show when the --verbose flag is present (#77), it's not critical that we have perfect parity with IPython's oinspect.info function. We can add in the edge cases in follow up changes. We do want to make sure we do a good job of getting docstrings though, since those will show up even outside of --verbose mode.