Improve IPython extension error handling #1761
Conversation
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
antonymilne
changed the title
| @@ -62,18 +56,18 @@ def my_register_pipeline(): | |||
| mocker.patch( | |||
| "kedro.framework.startup.bootstrap_project", return_value=fake_metadata | |||
| ) | |||
| mock_line_magic = mocker.MagicMock() | |||
| mock_line_magic = mocker.Mock() | |||
There was a problem hiding this comment.
Just curious is this necessary? Or we just prefer Mock in general. I think we use MagicMock quite a lot but without actually needing it in some tests.
There was a problem hiding this comment.
Good question - I did a quick survey of our use of MagicMock vs. Mock and found that nearly always it's Mock apart from in this file. There's no need for the extra capability of MagicMock here so I changed them all.
There was a problem hiding this comment.
Great catch! I think this is a small but significant change. I just fall into this trap this morning😅
When using kedro ipython/jupyter, these error messages appear on the terminal rather than in the Jupyter notebook itself. There's no easy way to push them to the Jupyter notebook. However, they can be reproduced there by executing %reload_kedro. I've added a note to the documentation to explain this. In the case of managed Jupyter instances this won't be a problem at all because you'll need to execute %load_ext in your notebook anyway, so the error will be surfaced there automatically.
I also checked this recently and couldn't find a way to do it, which is a bit annoying as almost 99% of people won't look at the terminal when they are using notebook.
There was a problem hiding this comment.
Great PR description as always! ⭐ I've got some questions, but the overall changes look good to me. It's slightly annoying the error messages don't show in the notebook, but I think your comment in the docs is very clear and I agree we can always populate the variables with Exceptions if users don't find the error handling clear.
Co-authored-by: Merel Theisen <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
…nto bugfix/ipython-error-handling Signed-off-by: Antony Milne <[email protected]> Signed-off-by: Antony Milne <[email protected]>
Description
Resolves #1590.
There were two problems with how the IPython extension was handling errors:
ImportErrorandModuleNotFounderror was meant to notify the user about when they didn't have Kedro installed but was too broad. The same error (Kedro appears not to be installed in your current environment) would also be raised when other dependencies (e.g. pandas) weren't installed, which was very confusingWARNING Kedro extension was registered but couldn't find a Kedro project. This is also very confusing because e.g. if you have a syntax error in your catalog.yml file you would have no idea what the problem was and instead think you were just in the wrong directoryWhat happens now?
If the Kedro project can't be found, a warning is emitted saying so:
Any other exception will be shown as is.
e.g. dataset dependency not installed:
e.g. catalog.yml syntax error:
Slight catch
When using
kedro ipython/jupyter, these error messages appear on the terminal rather than in the Jupyter notebook itself. There's no easy way to push them to the Jupyter notebook. However, they can be reproduced there by executing%reload_kedro. I've added a note to the documentation to explain this. In the case of managed Jupyter instances this won't be a problem at all because you'll need to execute%load_extin your notebook anyway, so the error will be surfaced there automatically.This is perfectly satisfactory for now I think. I also considered:
startup_errorvariable which captures the exception and is available in Jupyter notebooks. This is actually what we used to do before the IPython extension was created and we could put it back. But all the cases it solves are already covered by running%reload_kedroin your notebook and it doesn't seem any more discoverable than thatcatalog,context, etc. variables withExceptions by default, so that if they're not available then displaying them will explain why. This seems like overkill for now but could be done in future if people find the current system not clear enough. From observing what current user problems are, I think what we have here solves 99% of the problems.Checklist
RELEASE.mdfile