-
Notifications
You must be signed in to change notification settings - Fork 926
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
Remove the init_kedro
line magic from the Kedro IPython extension
#1349
Conversation
Signed-off-by: lorenabalan <[email protected]>
Signed-off-by: lorenabalan <[email protected]>
Signed-off-by: lorenabalan <[email protected]>
Signed-off-by: lorenabalan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Nice that we got rid of one of the globals as well 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! ⭐
I've left some minor comments which I know is on stuff outside the scope of this ticket, so please feel totally free to not implement them and merge this without changing anything. It would be useful though if you could say if you agree with the comments or not - if you're happy with the changes but don't want to make them here I can stick them in my next PR touching ipython stuff instead.
ipython.register_magic_function(reload_kedro, "line", "reload_kedro") | ||
|
||
project_path = _find_kedro_project(startup_path) | ||
default_project_path = _find_kedro_project(Path.cwd()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See following comment on error handling before this. If you agree with what I say there then this one is redundant...
Minor: If _find_kedro_project
can't find a kedro project and returns None
then this sets the default_project_path
to None
and will give the message "No path argument was provided. Using: None".
To me it would just feel a bit more natural if we used the None
value to specifically mean "reload_kedro
should re-use the current default_project_path
" and instead just passed Path.cwd()
in here instead. It's the default value of default_project_path
set on L14 so seems a bit odd to change it to None
rather than just keeping that default.
default_project_path = _find_kedro_project(Path.cwd()) | |
kedro_project path = _find_kedro_project(Path.cwd()) | |
default_project_path = kedro_project_path or Path.cwd() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's actually a fair point, I'll leave it to use cwd over None.
try: | ||
reload_kedro(project_path) | ||
reload_kedro(default_project_path) | ||
except (ImportError, ModuleNotFoundError): | ||
logging.error("Kedro appears not to be installed in your current environment.") | ||
except Exception: # pylint: disable=broad-except | ||
logging.warning( | ||
"Kedro extension was registered. Make sure you pass the project path to " | ||
"`%reload_kedro` or set it using `%init_kedro`." | ||
"Kedro extension was registered but couldn't find a Kedro project. " | ||
"Make sure you run `%reload_kedro <path_to_kedro_project>`." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: maybe this made sense before but it feels like a weird way of handling the flow now. If I try loading the extension and don't have kedro installed then I'll get "ModuleNotFoundError: No module named 'kedro.extras'" which is pretty clear already so I don't see the need to catch the ImportError
at all here? And then putting everything else in a broad except and assuming that's because a kedro project can't be found seems odd given we have a special value None
that signifies exactly that already.
To me this would make more sense (combined it with my above suggestion and tidied the result up a bit) as something like this:
kedro_project_path = _find_kedro_project(Path.cwd())
if kedro_project_path is not None:
reload_kedro(kedro_project_path)
else:
logging.warning(
"Kedro extension was registered but couldn't find a Kedro project. "
"Make sure you run `%reload_kedro <path_to_kedro_project>`."
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read this correctly, then it's not actually true. None
is returned if it finds a kedro project, not if kedro is installed. The import and module errors can still be raised if there IS a kedro project, but kedro is not installed (wrong activated environment for instance) in which case we give a more friendly errr message to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically if someone has the kedro ipython extension loaded (e.g.set in the ipython config), it shouldn't prevent them from using ipython at all, if they're just working in a completely different context. That's why we catch the exception and move on, but give them a heads up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the spirit of this principle, merging this as is as it's already gone a little out of scope, and final perfecting/refactoring can be done under a different task, as that will also require updating unit tests..
Signed-off-by: lorenabalan <[email protected]>
Description
Fixes #1336
reload_kedro
called with a path now also sets that path as default.reload_kedro
called on its own relies on the previously set default. The very first default is eitherNone
(if the extension was loaded in a notebook started in a non-kedro location) or the path to a kedro project recursively discovered from cwd (through_find_kedro_project
).Development notes
startup_path
globalproject_path
todefault_project_path
so that it's more obvious why we use if everywhere instead ofpath
init_kedro
and merged its defaulting mechanism intoreload_kedro
Testing
Running
in a location that's not a kedro project gives the following output
Running
%reload_kedro <path_to_a_non_kedro_project>
(and%reload_kedro
after that in the same session) gives the following error:Checklist
RELEASE.md
file