Make IPython optional #125

Nodd · 2022-02-20T20:52:17Z

The only runtime dependency is IPython, which needs many other libraries as well. This make a heavy installation for a features that is completely optional.
It was raised before in rkern/line_profiler#112, with some PR that were never merged, apparently due to lack of time: rkern/line_profiler#139, rkern/line_profiler#114. There is also comments from @rkern that it should be optional: rkern/line_profiler#90
This PR is my take on the subject, since I don't use ipython I had the motivation to get rid of it. It is still an optional dependency for pip, to be able to check the version: pip isntall line_profiler[ipython]

I think that the approach in rkern/line_profiler#139 is better: the IPython import is only made in the load_ipython_extension call. But since LineProfilerMagics is added to the "public API" in __init__.py I couldn't use it easily. The question is, can it be removed or is it needed somewhere ?

codecov · 2022-02-20T21:00:34Z

Codecov Report

Merging #125 (007efb7) into main (94147b3) will decrease coverage by 5.14%.
The diff coverage is 45.94%.

@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
- Coverage   53.96%   48.81%   -5.15%     
==========================================
  Files           4        5       +1     
  Lines         378      381       +3     
  Branches       58       58              
==========================================
- Hits          204      186      -18     
- Misses        156      172      +16     
- Partials       18       23       +5

Impacted Files	Coverage Δ
line_profiler/ipython_extension.py	`42.02% <42.02%> (ø)`
line_profiler/__init__.py	`100.00% <100.00%> (ø)`
line_profiler/line_profiler.py	`54.43% <100.00%> (+12.91%)`	⬆️
kernprof.py	`43.06% <0.00%> (-30.47%)`	⬇️

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 94147b3...007efb7. Read the comment docs.

Erotemic · 2022-02-20T22:26:55Z

I'm in favor of this change. At the very least, separating the IPython plugin from the core functionality is a positive step. Making it completely optional - IMO - is even better. But this is a widely used library, so let's be careful.

Based on a quick google search, I don't see any other projects that are using LineProfilerMagics directly, even though it is technically in the top-level API. I can see two paths forward:

Simply remove LineProfilerMagics from the top level API. It is unlikely that this will break anyone, and if it does they can raise an issue here, or just change where they import LineProfilerMagics from.
Keep the LineProfilerMagics in the top level API. This is safer. This might be the better way to go just in case. Instead of conditionally adding LineProfilerMagics to __all__. I would recommend, setting LineProfilerMagics = None in the case where the IPython extension fails to load.

I have a few more comments. I'll leave them inline.

Erotemic · 2022-02-20T22:31:42Z

line_profiler/line_profiler.py

-from IPython.utils.ipstruct import Struct
-from IPython.core.error import UsageError
+try:
+    from ipython_extension import load_ipython_extension


Should, at the very least, be from .ipython_extension import load_ipython_extension

However, this is a circular import, and while that can be ok, I'd rather avoid it for code readability. If I understand correctly, the important thing is that there is a load_ipython_extension function exposed at the top level.

Indeed, I forgot the leading dot, thanks for noticing.
I removed the circular import by not importing load_ipython_extension in line_profiler.py at all.

Erotemic · 2022-02-20T22:32:46Z

line_profiler/line_profiler.py

@@ -155,6 +155,8 @@ def add_module(self, mod):
        return nfuncsadded


+# This could be in the ipython_extension submodule,
+# but it doesn't depend on the IPython module so it's easier to just let it stay here.


I agree, this is just handling the case where we are printing something in a notebook. It doesn't have to do with the plugin directly, so it's fine to stay here.

Erotemic · 2022-02-20T22:35:16Z

line_profiler/__init__.py

           'load_ipython_extension', 'load_stats', 'main', 'show_func',
           'show_text', '__version__']
+
+try:
+    from .ipython_extension import LineProfilerMagics


In the else you can simply set LineProfilerMagics = None to avoid having to dynamically modify __all__ (I prefer it when __all__ is static). We can do the same for the load_ipython_extension function, but maybe it becomes a stub function that raises an error if IPython is not available?

I changed the way LineProfilerMagics is defined and added a lazy import. I still think LineProfilerMagics should be removed from __init__.py as an implementation detail, but I understand your concern about potential backward compatibility.

I would be ok with removing this from the top level API. It doesn't really belong there, and again I doubt anyone is using it. It might be good to search for any potential usage over github and other OSS code repos just in case though. If it doesn't look like it is in use, let's get rid of it if that's easier.

Nodd · 2022-02-21T00:33:46Z

With the last modifications, IPython is not imported if it's installed but line_profiler is not used as an extension.

Nodd · 2022-02-21T07:17:36Z

line_profiler/__init__.py

                            show_func, show_text,)

+from .ipython_extension import load_ipython_extension, LineProfilerMagics


This won't work with lazy import, at this point LineProfilerMagics is always None, and it won't change when the value is updated in ipython_extension.

Will it be the case that we only care if load_ipython_extension is defined if IPython is in sys.modules? That could be a potential optimization that prevents the large import in cases where it is not needed.

Good idea, it will be cleaner this way and should work with you other suggestion below.

Erotemic · 2022-02-21T16:27:14Z

line_profiler/ipython_extension.py

+    """ API for IPython to recognize this module as an IPython extension.
+    """
+
+    # Import IPython inside the function to load IPython only if necessary


I definitely don't want to define this class as a local object. That can cause weird corner cases on win32 where objects need to be defined in the global scope for them to be pickleable, and hence shared across a multiprocessing queue. An easy workaround is to make an _ipython_extension_backend.py module (If the ipython_extension.py module can't serve that purpose) that defines the classes of interest, but is only conditionally imported.

I see what you mean, I'll try something like this.

Nodd · 2022-02-21T21:04:21Z

line_profiler/line_profiler.py

+def load_ipython_extension(ip):
+    """ API for IPython to recognize this module as an IPython extension.
+    """
+    from .ipython_extension import LineProfilerMagics


It can either be imported from this function, or it can be added with the other imports with a sys.modules guard for IPython.
In the second case there would be a circular import but it shouldn't be a problem.

Nodd · 2022-02-21T21:06:32Z

There should probably be some tests to check the behavior with and without IPython but I'm not sure how to do it.

Erotemic · 2022-02-22T02:43:49Z

Tests for a larger change like this would be ideal.

I found a few articles that look like they go through how to test ipython magics:

https://pmbaumgartner.github.io/blog/testing-ipython-magics/

https://medium.com/@davide.sarra/how-to-test-magics-in-ipython-extensions-86d99e5d6802

Nodd · 2022-03-05T23:54:52Z

Thanks to the links you provided I added a test for IPython.
I don't understand the coverage results for ipython_extension.py, the whole lprun() function is run (the output is tested) but the coverage is not detected after line 104. I have the same result locally. I don't understand what is happening.

Erotemic · 2022-03-06T23:41:56Z

There has been problems with this repo and coverage. I'm not sure of the ultimate culprits either, but that's a known issue and does not need to be part of this PR. Right now it LGTM, I'll do a final more careful review in the next few days and then merge.

Erotemic · 2022-03-17T15:34:26Z

Thanks @Nodd, this LGTM!

This reduces import time from 168,715 to 10,893 microseconds. 10x improvement!

Nodd · 2022-03-20T23:38:42Z

Nice !
It should also speed up the unittests for lineprofilergui, since the slowest steop is the dependency installation.

Erotemic reviewed Feb 20, 2022

View reviewed changes

Nodd force-pushed the ipython_optional branch from 163f8fd to cf4fd38 Compare February 21, 2022 00:12

Nodd and others added 7 commits February 21, 2022 01:15

IPython is now optional

4a55255

Manage imports in __init__.py

cb13a77

Forgot local import

6f92dca

Use load_ipython_extension from line_profiler to get a useful message

53c5c59

Typo

d547295

Rework IPython plugin layout

dd4c425

Update CHANGELOG

adfc2cb

Nodd force-pushed the ipython_optional branch from cf4fd38 to adfc2cb Compare February 21, 2022 00:16

Merge branch 'main' into ipython_optional

e5f988e

Nodd commented Feb 21, 2022

View reviewed changes

Erotemic reviewed Feb 21, 2022

View reviewed changes

Load IPython only when needed

5de7781

Nodd commented Feb 21, 2022

View reviewed changes

Nodd and others added 3 commits February 23, 2022 23:28

Add basic ipython test

56e5a74

Fix deprecation warning

583cf27

Check output

007efb7

Erotemic merged commit 8c7624c into pyutils:main Mar 17, 2022

Nodd deleted the ipython_optional branch March 20, 2022 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make IPython optional #125

Make IPython optional #125

Nodd commented Feb 20, 2022 •

edited

Loading

codecov bot commented Feb 20, 2022 •

edited

Loading

Erotemic commented Feb 20, 2022

Erotemic Feb 20, 2022

Nodd Feb 21, 2022

Erotemic Feb 20, 2022

Erotemic Feb 20, 2022

Nodd Feb 21, 2022

Erotemic Feb 21, 2022

Nodd commented Feb 21, 2022

Nodd Feb 21, 2022

Erotemic Feb 21, 2022

Nodd Feb 21, 2022

Erotemic Feb 21, 2022

Nodd Feb 21, 2022

Nodd Feb 21, 2022

Nodd commented Feb 21, 2022

Erotemic commented Feb 22, 2022

Nodd commented Mar 5, 2022 •

edited

Loading

Erotemic commented Mar 6, 2022

Erotemic commented Mar 17, 2022 •

edited

Loading

Nodd commented Mar 20, 2022

		show_func, show_text,)

		from .ipython_extension import load_ipython_extension, LineProfilerMagics

Make IPython optional #125

Make IPython optional #125

Conversation

Nodd commented Feb 20, 2022 • edited Loading

codecov bot commented Feb 20, 2022 • edited Loading

Codecov Report

Erotemic commented Feb 20, 2022

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

Nodd commented Feb 21, 2022

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

Nodd commented Feb 21, 2022

Erotemic commented Feb 22, 2022

Nodd commented Mar 5, 2022 • edited Loading

Erotemic commented Mar 6, 2022

Erotemic commented Mar 17, 2022 • edited Loading

Nodd commented Mar 20, 2022

Nodd commented Feb 20, 2022 •

edited

Loading

codecov bot commented Feb 20, 2022 •

edited

Loading

Nodd commented Mar 5, 2022 •

edited

Loading

Erotemic commented Mar 17, 2022 •

edited

Loading