Remove .ipython dir and introduce kedro Jupyter kernel #1355

antonymilne · 2022-03-18T17:20:49Z

Note for reviewers

The diff here isn't as bad as the number of files changed makes it look! Much of it is just deleting old files. The interesting files to review are just:

kedro/framework/cli/jupyter.py
kedro/framework/cli/project.py

The docs have changed substantially though - best way to review that is not to look at the .md file but instead to check the rendered version.

Description

Closes #1338. In brief, this changes kedro jupyter notebook/lab and kedro ipython commands so that instead of using the .ipython folder, they now use the kedro.extras.extensions.ipython extension instead. This ensures that there is one consistent way of using kedro interactively.

kedro ipython is basically equivalent to ipython --ext=kedro.extras.extensions.ipython. kedro jupyter notebook/lab achieve the same thing (i.e. automatically loading the extension) by creating a custom Jupyter kernel which points to an ipython launcher that includes --ext=kedro.extras.extensions.ipython.

There's no kedro command to remove the kernel; you just do jupyter kernelspec remove kernel_name. Note that all kernels created by kedro have the kedro_ prefix so you can immediately see where they come from if you do jupyter kernelspec list. This stuff will go in the docs when I write them.

One advantage of a custom kernel is we now have a snazzy icon on the UI (update! The icon is now the black version as in the docs but I didn't change these screenshots)

Jupyter notebook

Jupyter lab

Development notes

I've needed to make lots of small decisions here exactly what to change and how big a breaking change this should be. Overall I've tried to balance up the opportunity to do a breaking release + wanting to achieve one consistent workflow + ensure that stuff still works + not being overly zealous in removing things that we're not yet sure should be removed. I'll make a separate ticket for items which could be revisited in future. Please let me know if you disagree with any of these decisions since they're very much open for discussion.

Things I've changed here

Removed ipython_message message informing users which variables are in scope. This seemed to be very out of date and no longer needed given the very similar message printed by load_ipython_extension
Removed ip argument for kedro jupyter notebook/lab. This actually won't affect anything because you can still do kedro jupyter notebook/lab --ip=xxx and it will get passed through *args
Removed idle_timeout argument for kedro jupyter notebook/lab. This was a tricky one to decide on because it was originally introduced for a good reason: to fix a bug in which the %run_viz line magic creates a process that blocks port 4141 even after Jupyter is killed. However, (a) the fix does not seem to work for me [i.e. %run_viz still blocks port 4141 even with the fix], (b) this fix can't ever work on managed jupyter instances so is incompatible with our goal of "one consistent workflow", (c) does anyone use %run_viz anyway? So overall I decided to remove the fix and then later let's work out whether it's really an issue and if so come up with a different fix (probably on the viz side, a %kill_viz command or something).
Removed the SingleKernelSpecManager that ensured the user only had one kernel to choose from. This is incompatible with the one consistent workflow goal and seemed unnecessary now that we are clearly labelling the Kedro kernel and making it the default. If we do want to reintroduce this behaviour then it might now be easier to do through KernelSpecManager.allowed_kernelspecs rather than making a whole custom kernel spec manager
Removed all_kernels option which gave the user the possibility of overriding the SingleKernelSpecManager to show all available kernels (since this is now how it always behaves)

Things I've not changed here

KEDRO_ENV environment variable remains. This provides a mechanism for changing env and is picked up when the session is created. As far as I can see, the interactive workflow doesn't strictly need this since a user can do %reload_kedro env=xxx to change environment. However, since removing this would in theory affect the creation of sessions outside the interactive workflow also I left it as is. I think we should check with users whether and how it's used and then hopefully remove it in future.
Calls to validate_settings remain in kedro jupyter notebook/labs. I don't know if these are still needed but I didn't want to risk taking them out and breaking stuff. If they are still needed then maybe these calls should move to kedro.extensions.extras.ipython in the future

Checklist

Read the contributing guidelines
Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added a description of this change in the RELEASE.md file
Added tests to cover my changes

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

kedro/framework/cli/jupyter.py

datajoely · 2022-03-18T20:30:48Z

Nice! Would just add I think the KEDRO_ENV env var has a lot of value outside of the interactive workflow

antonymilne · 2022-03-18T22:17:48Z

kedro/templates/project/{{ cookiecutter.repo_name }}/src/requirements.txt

@@ -1,6 +1,5 @@
 black==21.5b1
 flake8>=3.7.9, <4.0
-ipython~=7.10


This was just left over from a bad merge - the specification on the next line is the right one.

antonymilne · 2022-03-18T22:18:39Z

requirements.txt

@@ -6,7 +6,6 @@ dynaconf>=3.1.2,<4.0.0
 fsspec>=2021.4, <=2022.1
 gitpython~=3.0
 jmespath>=0.9.5, <1.0
-jupyter_client>=5.1, <8.0


This is already in the project template requirements, which is where it really belongs.

As noted elsewhere I've actually removed jupyter_client altogether:

This is a core dependence of jupyter so I've decided to not list it explicitly so we don't have to keep bumping the version requirements.

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne · 2022-03-21T09:00:18Z

kedro/framework/cli/jupyter.py

+    )
+
+
+def _create_kernel(kernel_name: str, display_name: str) -> None:


This is the bit that might eventually become its own kedro jupyter-init or something like that.

I spent a loooong time reading through the ipython and jupyter source to try and figure out which were the right high-level API commands to use here to make this as robust as possible so we're not relying on too much stuff that might change. So🤞 this carries on working in the future, but I decided to the wrap the whole call in try/except just in case...

idanov

It's pretty nice to see this change happening finally 🎉 I think we need to get rid of all references to the .ipython/ directory in our codebase and docs, as well as in all our official starters.

edit: I saw you already plan to do that, so my comment here is unneeded. Btw could we have the dark Kedro icon instead? It stands out better in the white-background theme in Jupyter.

kedro/templates/project/{{ cookiecutter.repo_name }}/.ipython/profile_default/ipython_config.py

idanov · 2022-03-21T09:15:38Z

kedro/templates/project/{{ cookiecutter.repo_name }}/src/requirements.txt

@@ -1,6 +1,5 @@
 black==21.5b1
 flake8>=3.7.9, <4.0
-ipython~=7.10
 ipython>=7.31.1, <8.0


Could we relax this one further, e.g. <9.0? The most recent ipython version atm is 8.1.1.

We could, but ipython 8 requires Python >= 3.8 and we still support 3.7. Happy to bump it though if you like and let pip work out which version to install for the user's given version of Python. Or maybe even just remove it from the requirements given it's a core dependency of jupyter and jupyterlab anyway?

I think if we can support both (i.e. no breaking changes that make us have different code for different python versions), we can bump the requirement. I'm slightly more in favour of that over removing it completely, just for users that want to use ipython over jupyter and only need to remove the relevant lines. I don't have any strong opinions on this though.

kedro/framework/cli/jupyter.py

idanov · 2022-03-21T09:27:05Z

kedro/framework/cli/jupyter.py

+    kernel_spec = json.loads(kernel_json.read_text())
+    kernel_spec["argv"].extend(["--ext", "kedro.extras.extensions.ipython"])
+    # indent=1 is to match the default ipykernel style (see ipykernel.write_kernel_spec).
+    kernel_json.write_text(json.dumps(kernel_spec, indent=1))


Could we use json.dump here instead?

Not while using pathlib (which is what we generally use now), because json.dump needs a file pointer rather than a path object.

tests/framework/cli/test_jupyter.py

tests/framework/cli/test_project.py

lorenabalan

LGTM, but a little worried about the line magic not working.

kedro/framework/cli/jupyter.py

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne · 2022-03-25T16:56:55Z

OK, we're good to go here! Updating the docs turned into quite a substantial rewrite because a lot of the page was out of date. If you want to review the new docs the easiest way is to look at the rendered version here: https://antonymilneqb.github.io/tools_integration/ipython.html

Still to do:

update release notes: see Update release notes with Jupyter/IPython changes #1373
make new tickets out of some small issues discovered here
update kedro-starters, but only once this PR has merged

antonymilne · 2022-03-25T17:07:37Z

features/steps/test_starter/{{ cookiecutter.repo_name }}/src/requirements.txt

@@ -3,7 +3,6 @@ flake8>=3.7.9, <4.0
 ipython>=7.31.1, <8.0
 isort~=5.0
 jupyter~=1.0
-jupyter_client>=5.1, <8.0


This is a core dependence of jupyter so I've decided to not list it explicitly so we don't have to keep bumping the version requirements.

antonymilne · 2022-03-25T17:10:38Z

setup.py

@@ -119,7 +121,6 @@ def _collect_requirements(requires):
        "ipykernel>=5.3, <7.0",
    ],
    "geopandas": _collect_requirements(geopandas_require),
-    "ipython": ["ipython>=7.31.1, <8.0"],


I removed this just to simplify the number of places we specify dependencies, and because we have never advertised pip install kedro[ipython] as a thing. There's already helpful messages when you do kedro jupyter/ipython if you don't have the right things installed.

antonymilne · 2022-03-25T17:12:05Z

tests/framework/cli/test_jupyter.py

+
+@pytest.mark.usefixtures("cleanup_kernel")
+class TestCreateKernel:
+    def test_create_new_kernel(self):


I decided to do this as a sort of e2e test that checks what the actual result of calling _create_kernel is rather than mocking out all the calls inside that function. This means test is not tied to a particular implementation and will be easy to modify if and when _create_kernel becomes its own kedro jupyter-init command.

yetudada

This is great! I left minor comments 🚀

docs/source/tools_integration/ipython.md

merelcht · 2022-03-28T16:08:30Z

kedro/framework/cli/jupyter.py

    """Open Jupyter Notebook with project specific variables loaded."""
-    _check_module_importable("jupyter_core")
-
+    _check_module_importable("notebook")


It makes a lot more sense to me that this is now notebook, but I'm just wondering why was it jupyter_core before?

lorenabalan

Some small observations but LGTM!

setup.py

lorenabalan · 2022-03-29T09:09:49Z

kedro/templates/project/{{ cookiecutter.repo_name }}/src/requirements.txt

@@ -1,6 +1,5 @@
 black==21.5b1
 flake8>=3.7.9, <4.0
-ipython~=7.10
 ipython>=7.31.1, <8.0


I think if we can support both (i.e. no breaking changes that make us have different code for different python versions), we can bump the requirement. I'm slightly more in favour of that over removing it completely, just for users that want to use ipython over jupyter and only need to remove the relevant lines. I don't have any strong opinions on this though.

docs/source/tools_integration/ipython.md

merelcht

Forgot to approve earlier, my question shouldn't be a blocker. Great work 👍

Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>

antonymilne added 10 commits March 18, 2022 10:16

Tidy requirements.txt

21c6c2b

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Tidy requirements.txt

caaead1

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Modify ipython command

6523d4d

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Remove ipython_loader

11d7450

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Simplify jupyter notebook commands

f8ecf3f

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Further simplify and remove ipython_message

778c85a

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Add kernel installation

dc5bb19

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Switch from MappingKernelManager to MultiKernelManager

f39bf2e

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Add message for when kernel already exists

a782657

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Add comment

4de7d1c

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne commented Mar 18, 2022

View reviewed changes

kedro/framework/cli/jupyter.py Show resolved Hide resolved

antonymilne commented Mar 18, 2022

View reviewed changes

Lint

32911be

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne requested a review from lorenabalan March 18, 2022 22:35

antonymilne commented Mar 21, 2022

View reviewed changes

idanov reviewed Mar 21, 2022

View reviewed changes

lorenabalan reviewed Mar 21, 2022

View reviewed changes

antonymilne added 11 commits March 24, 2022 11:08

Add to MANIFEST and update API docs

a82bc70

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Move try/except inside _create_kernel

e88bd82

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Correct imports checks

7c05f9b

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

ipython -> IPython

fa5f5f3

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Tidy requirements

87a3849

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Fix jupyter notebook tests

d0e82cc

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Fix jupyter lab tests and add _create_kernel tests

35868bb

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Fix stuff

292b0eb

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Update logos

2846f02

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Fix more stuff

fbec113

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

Start changes to docs

6d08da8

Signed-off-by: Antony Milne <antony.milne@quantumblack.com>

antonymilne requested review from lorenabalan, merelcht and idanov March 25, 2022 16:58

antonymilne commented Mar 25, 2022

View reviewed changes

antonymilne mentioned this pull request Mar 28, 2022

Update release notes with Jupyter/IPython changes #1373

Merged

5 tasks

yetudada approved these changes Mar 28, 2022

View reviewed changes

merelcht reviewed Mar 28, 2022

View reviewed changes

lorenabalan approved these changes Mar 29, 2022

View reviewed changes

merelcht approved these changes Mar 29, 2022

View reviewed changes

lorenabalan added 4 commits March 30, 2022 10:45

Apply review suggestions

b49142f

Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>

Bump ipython

6803d70

Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>

Add link to Yetu's talk

25fcd62

Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>

Merge branch 'develop' into remove-ipython-dir

d6ee5bf

Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>

lorenabalan force-pushed the remove-ipython-dir branch from be93e2a to d6ee5bf Compare March 30, 2022 10:19

lorenabalan added 3 commits March 30, 2022 12:57

Undo ipython bump

443e99d

Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>

Fix lint

a733323

Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>

Fix test

dc8bec9

Signed-off-by: lorenabalan <lorena.balan@quantumblack.com>

lorenabalan merged commit 8c899a9 into develop Mar 30, 2022

lorenabalan deleted the remove-ipython-dir branch March 30, 2022 13:33

lorenabalan linked an issue Mar 30, 2022 that may be closed by this pull request

Make kedro ipython/jupyter commands not use the project's .ipython/ dir #1338

Closed

6 tasks

lorenabalan mentioned this pull request Mar 30, 2022

Make kedro ipython/jupyter commands not use the project's .ipython/ dir #1338

Closed

6 tasks

merelcht mentioned this pull request Mar 30, 2022

Remove .ipython dir and update readme files kedro-org/kedro-starters#78

Merged

3 tasks

antonymilne mentioned this pull request Apr 5, 2022

%run_viz line magic requires session.close first kedro-org/kedro-viz#811

Closed

antonymilne mentioned this pull request May 19, 2022

Fix %run_viz kedro-org/kedro-viz#855

Closed

antonymilne mentioned this pull request Jun 15, 2022

New platform-dependent %launch_viz line magic kedro-org/kedro-viz#910

Closed

5 tasks

antonymilne mentioned this pull request Sep 8, 2022

Should we remove KEDRO_ENV? #1842

Closed

5 tasks

rxm7706 mentioned this pull request Jun 19, 2023

Remove python-json-logger and jupyter_Client from conda run requirements conda-forge/kedro-feedstock#37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove .ipython dir and introduce kedro Jupyter kernel #1355

Remove .ipython dir and introduce kedro Jupyter kernel #1355

antonymilne commented Mar 18, 2022 •

edited

Loading

datajoely commented Mar 18, 2022

antonymilne Mar 18, 2022

antonymilne Mar 18, 2022

antonymilne Mar 25, 2022

antonymilne Mar 21, 2022

idanov left a comment •

edited

Loading

idanov Mar 21, 2022

antonymilne Mar 24, 2022

lorenabalan Mar 29, 2022

idanov Mar 21, 2022

antonymilne Mar 25, 2022

lorenabalan left a comment •

edited

Loading

antonymilne commented Mar 25, 2022 •

edited by merelcht

Loading

antonymilne Mar 25, 2022

antonymilne Mar 25, 2022

antonymilne Mar 25, 2022 •

edited

Loading

yetudada left a comment

merelcht Mar 28, 2022

lorenabalan left a comment

lorenabalan Mar 29, 2022

merelcht left a comment

		)


		def _create_kernel(kernel_name: str, display_name: str) -> None:

Remove .ipython dir and introduce kedro Jupyter kernel #1355

Remove .ipython dir and introduce kedro Jupyter kernel #1355

Conversation

antonymilne commented Mar 18, 2022 • edited Loading

Note for reviewers

Description

Development notes

Things I've changed here

Things I've not changed here

Checklist

datajoely commented Mar 18, 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

idanov left a comment • edited Loading

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

lorenabalan left a comment • edited Loading

Choose a reason for hiding this comment

antonymilne commented Mar 25, 2022 • edited by merelcht Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonymilne Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

yetudada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenabalan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merelcht left a comment

Choose a reason for hiding this comment

antonymilne commented Mar 18, 2022 •

edited

Loading

idanov left a comment •

edited

Loading

lorenabalan left a comment •

edited

Loading

antonymilne commented Mar 25, 2022 •

edited by merelcht

Loading

antonymilne Mar 25, 2022 •

edited

Loading