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

Alias IPython extension #1763

Closed
antonymilne opened this issue Aug 5, 2022 · 6 comments · Fixed by #1837
Closed

Alias IPython extension #1763

antonymilne opened this issue Aug 5, 2022 · 6 comments · Fixed by #1837
Assignees

Comments

@antonymilne
Copy link
Contributor

I'm fed up of (mis)typing %load_ext kedro.extras.extensions.ipython. Let's make everyone's lives a bit easier and alias it.

Previously we had said that %load_ext kedro.ipython would be an improvement, but I'm going to make a drastic suggestion and go even further to suggest %load_ext kedro. If you're doing %load_ext anyway then you know it's an IPython extension so the ipython part is just unnecessary extra characters.

How?

Really just one line of code in kedro/__init__.py that says:

from .extras.extensions.ipython import load_ipython_extension

This should turn the module kedro into an IPython extension without affecting any of the existing kedro functionality.

Is that it?

No! This is the important part. As per discussion we had as part of #712 we need to make sure that this import doesn't have undesired side effects, since the above line of code will run every time anyone imports anything from kedro. This means that any imports in the extras -> extensions -> ipython import chain will also be executed.

There are three possible risks here:

  • the import tries to import something that is not a core dependency of kedro and the user doesn't have (which includes IPython!)
  • the import imports loads of things that exist but makes things very slow
  • the import has side effects that we don't want to be executed (e.g. importing from kedro.framework.project import LOGGING actually configures logging)

All three risks here should be mitigated against because in #1761 I already hid all the non-standard library imports inside functions as a guard. So, unless I missed something, what you need to do is write some tests to ensure that this does not get accidentally broken in the future. Something like these (will need some cunning mocking):

  • import kedro does not try to import IPython
  • import kedro does not do imports like from kedro.framework.project import LOGGING

Obviously manually test all this too, but the important thing here is writing the automated tests so it doesn't break in future if someone moves an import to be unguarded at the top level of the ipython.py file.

Anything else to do?

Yes! Update all mentions in the codebase of kedro.extras.extensions.ipython to kedro. This is probably just docs and the kedro ipython/jupyter commands and should be easy. The hard thing here is the writing the tests.

@yetudada
Copy link
Contributor

yetudada commented Aug 5, 2022

I love this! This would be a great UX improvement. Might I suggest that it's %load_ext ipython instead? I don't know why but this feels weird:

%load_ext kedro
%reload_kedro

@antonymilne
Copy link
Contributor Author

antonymilne commented Aug 5, 2022

@yetudada that won't work because we don't have a package called ipython (that's owned by IPython). %load_ext X is not something from kedro; it's the general IPython command to load the IPython extension called X. So if you have a library X then you might call your IPython extension X or X-ipython-extension or something like that.

So %load_ext kedro would tell IPython to load up our kedro IPython extension, which registers the line magic %reload_kedro and variables catalog, context, etc.

@noklam
Copy link
Contributor

noklam commented Aug 22, 2022

@AntonyMilneQB Would %load_ext kedro.ipython be worse? I think the current approach is fine, but also want to check if we want to take this opportunity to remove the extras.

Currently we have extras.extensions.ipython, extras.logging(would be removed), extras.datasets (likely to be removed). Do we still want to keep this kedro.extras namespace?

@antonymilne
Copy link
Contributor Author

I think %load_ext kedro.ipython is not as good as %load_ext kedro because it's not as short and clear. If you're doing %load_ext then you know you're trying to load an IPython extension, so the extra .ipython seems redundant to me. Looking through a list of IPython extensions this seems like a common way to name them also, e.g. the SQLAlchemy IPython extensions is loaded as %load_ext sql and not %load_ext sql.ipython. wdyt?

As for removing extras entirely, absolutely would probably be good to do that - it's always been the vague intention after datasets have moved. This would need to be only in develop though. Also, in this case we need to figure out where exactly the stuff inside extras/extensions/ (which includes .png files) should live. Maybe a new directory kedro/ipython/ or maybe inside kedro/framework/ipython/?

The reason I say it would _probably be good to remove extras is that possibly in the future we would extend #1563 to move spark hooks (and maybe rich logging hooks?) to live inside the kedro package somewhere. So we might need to figure out where those belong also, and then maybe extras makes sense again? I don't know.

Overall, I think:

  • leave all the code where it is in main and just alias it with the from .extras.extensions.ipython import load_ipython_extension import
  • open a ticket to probably move it out of extras to somewhere else (for 0.19; should take into consideration the above point about other stuff that might exist)

@noklam
Copy link
Contributor

noklam commented Aug 22, 2022

@AntonyMilneQB I think %load_ext kedro makes more sense, my main concerns are the ones you mentioned, once we put it in the top kedro directory it's very hard to get rid of it since it gets triggered when importing any kedro submodule, but I don't see a way to get around this so I guess this is the only way.

To your 2nd point, I agree it's not something we should do now, a separate issue to keep this discussion alive is ideal👍🏼

@antonymilne
Copy link
Contributor Author

Yeah, in this case I don't think it's a problem that the import is triggered whenever we import anything from kedro. I thought of three possible risks here, as detailed in the top post, but I think none of them will be a problem here. Please do shout if there's any other risks I didn't think of though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment