Update documentation to show how to use `KedroSession` outside of CLI mode #1713

noklam · 2022-07-19T15:09:53Z

Signed-off-by: Nok Chan nok.lam.chan@quantumblack.com

Description

#1583 introduce a behavioral change that was introduced in 0.18.0, this PR updated the documentation to better reflect this.

Development notes

update the documentation to reflect when does a user need to do bootstrap_project

bonus:

a more helpful message when you are not running kedro in the right directory
removed the duplicate code in ipython extension

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: Nok Chan <nok.lam.chan@quantumblack.com>

noklam · 2022-07-19T15:55:54Z

kedro/extras/extensions/ipython.py

@@ -58,7 +58,6 @@ def reload_kedro(

    _remove_cached_modules(metadata.package_name)

-    configure_project(metadata.package_name)


The bootstrap_project method already included the configure_project, so I don't think it is needed

I've been meaning to do this for ages. I didn't do it before because at a glance I wasn't 100% sure whether it was intentional to have both bootstrap_project and configure_project here. Note there's a call to _remote_cached_modules in between - does that mean that actually we need to repeat the call to configure_project?

I'm very much in favour of removing this if we can, but please do double check to see that all still works correctly after removing it! Possibly there's a good reason it's there.

Sounds like you are confident we can remove this, so please go ahead as you were before 👍

noklam · 2022-07-19T16:23:21Z

Dev notes:

Intended to add some helpful message denoted that user is running outside of a kedro project - found that kedro-telemetry is initializing the KedroCLI again so it causes duplicate message.

https://github.com/kedro-org/kedro-plugins/blob/5813362d304babc3434f9d8e32e63478f8b95de7/kedro-telemetry/kedro_telemetry/plugin.py#L45

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam · 2022-07-20T16:28:35Z

Mostly base on @datajoely's magic, I added some more so it will attempt to find the project. If it finds a kedro project then it will print the suggested path.

The last commit could be done at a separate commit, but I find this is kinda related anyway since it's dealing with things break in "out of project" mode.

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

…into fix/docs_session_create Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

datajoely · 2022-07-20T16:45:16Z

Really nice work @noklam - would you mind providing some screenshots? I'll defer any points on implementation to the rest of the group, but would love to see the user journey here.

noklam · 2022-07-20T16:53:06Z

If users are in the project directory:
Business as usual, any kedro command should work

If users are not in the project directory:

'kedro' - should a message suggesting this is running outside of a project? (Not sure so i didn't change it) - it's unclear whether user are aware of the project commands, there are no error but it will just show the global command.
'kedro run' or any project command - it's pretty clear user are aware of these commands but just from wrong directory, so it will fail and show the screenshot above. If a project path is found, it will just show the path, if not it will revert to a simple help message suggesting user should look for the directory where the pyproject.toml

Typing this on my phone so sorry for badly formoatted.
@datajoely

What even better maybe it should just be a copyable message

cd is_this_your_project
kedro run

datajoely · 2022-07-20T16:57:26Z

I didn't realise that was a new screenshot, I thought it was mine from ages ago. Nice work!

antonymilne · 2022-07-21T08:10:49Z

kedro/extras/extensions/ipython.py

@@ -58,7 +58,6 @@ def reload_kedro(

    _remove_cached_modules(metadata.package_name)

-    configure_project(metadata.package_name)


I've been meaning to do this for ages. I didn't do it before because at a glance I wasn't 100% sure whether it was intentional to have both bootstrap_project and configure_project here. Note there's a call to _remote_cached_modules in between - does that mean that actually we need to repeat the call to configure_project?

I'm very much in favour of removing this if we can, but please do double check to see that all still works correctly after removing it! Possibly there's a good reason it's there.

kedro/framework/session/session.py

kedro/framework/project/__init__.py

antonymilne · 2022-07-21T08:34:54Z

kedro/framework/cli/cli.py

        self._cli_hook_manager = get_cli_hook_manager()

        super().__init__(
            ("Global commands", self.global_groups),
            ("Project specific commands", self.project_groups),
        )

+    def get_command(self, ctx: Context, cmd_name: str) -> Optional[Command]:


I'm a huge fan of this change in principle, thank you for adding it! However, please could you split it off into a separate PR? There's a few things I think need discussing:

not at all sure about using black for find_project_root. Note we have something similar _find_kedro_project in the ipython extension; if that seems suitable to reuse, let's move it to some common utils file and use it here

more importantly, if we are inside a kedro project, just not at the top level, is there actually any reason these project commands shouldn't work? 🤔 Instead of an error message telling the user to cd can we actually just somehow make kedro work inside a project subfolder (like kedro ipython does)? But without cding to the kedro project top-level

if we're totally outside a kedro project, a helpful error message is great but:

is there some better way to check if the command is a project one without overriding get_command, e.g. check if it's in self.project_groups?

I don't think this formatting is "on brand" yet since we haven't moved the rest of the CLI to rich yet. Let's just do it as click.secho for now. Doing rich styling for CLI messages is a separate issue for another time (e.g. we might move to https://github.com/ewels/rich-click)

#1720 Separate PR created here, I put it in the inbox so we can discuss it a bit more.

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

This reverts commit edda3f1. Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam · 2022-07-21T16:03:44Z

tests/extras/extensions/test_ipython.py

+PACKAGE_NAME = "fake_page_name"
+PROJECT_NAME = "fake_project_name"
+PROJECT_VERSION = "0.1"
+
+


just minor refactoring since these values appears a few time.

noklam · 2022-07-21T16:04:42Z

tests/extras/extensions/test_ipython.py

-        mocker.patch("kedro.framework.project.settings.configure")
-        mocker.patch("kedro.framework.session.session.validate_settings")
-        mocker.patch("kedro.framework.session.KedroSession._setup_logging")


@AntonyMilneQB Are these actually needed? I found that they are not in the test_load_kedro_objects_extra_args test and removing them seems to have no effect, I am not sure why it is needed.

Probably not needed 👍

noklam · 2022-07-21T16:04:53Z

tests/extras/extensions/test_ipython.py

@@ -67,16 +70,21 @@ def my_register_pipeline():
        mock_register_line_magic = mocker.patch(
            "kedro.extras.extensions.ipython.register_line_magic"
        )
-        mock_context = mocker.patch("kedro.framework.session.KedroSession.load_context")


We should use session instead

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

antonymilne · 2022-07-21T20:28:30Z

kedro/extras/extensions/ipython.py

@@ -58,7 +58,6 @@ def reload_kedro(

    _remove_cached_modules(metadata.package_name)

-    configure_project(metadata.package_name)


Sounds like you are confident we can remove this, so please go ahead as you were before 👍

antonymilne · 2022-07-21T20:29:06Z

tests/extras/extensions/test_ipython.py

-        mocker.patch("kedro.framework.project.settings.configure")
-        mocker.patch("kedro.framework.session.session.validate_settings")
-        mocker.patch("kedro.framework.session.KedroSession._setup_logging")


Probably not needed 👍

AhdraMeraliQB

Great work! Good catch on the test refactoring too!

Update doc

6c75eb3

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam requested a review from idanov as a code owner July 19, 2022 15:09

noklam marked this pull request as draft July 19, 2022 15:27

remove the extra configure_project

a25e4a9

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam commented Jul 19, 2022

View reviewed changes

noklam removed the request for review from idanov July 19, 2022 16:46

noklam added 2 commits July 20, 2022 17:26

docs update

7a54f26

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

Add more helpful cli message

edda3f1

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

Merge branch 'main' into fix/docs_session_create

d65f7a6

noklam requested review from antonymilne, datajoely and SajidAlamQB July 20, 2022 16:29

noklam marked this pull request as ready for review July 20, 2022 16:31

noklam added 2 commits July 20, 2022 17:32

fix some linting

7e350b1

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

Merge branch 'fix/docs_session_create' of github.com:kedro-org/kedro …

a75a0b6

…into fix/docs_session_create Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

antonymilne reviewed Jul 21, 2022

View reviewed changes

noklam and others added 4 commits July 21, 2022 11:24

Update kedro/framework/project/__init__.py

ca2bca9

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

Revert "Add more helpful cli message"

27463fa

This reverts commit edda3f1. Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

Update kedro/framework/session/session.py

79da3c4

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

Update kedro/framework/project/__init__.py

e7c7670

Co-authored-by: Antony Milne <49395058+AntonyMilneQB@users.noreply.github.com>

noklam mentioned this pull request Jul 21, 2022

Provide better error message when running project command like kedro run not from the root directory #1720

Closed

5 tasks

noklam linked an issue Jul 21, 2022 that may be closed by this pull request

Investigate KedroSession.create: ModuleNotFoundError: No module named 'None' #1583

Closed

noklam added 2 commits July 21, 2022 16:46

Fixing unittest

01923a5

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

FIX test

b88473e

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam commented Jul 21, 2022

View reviewed changes

noklam added 2 commits July 21, 2022 17:29

Fix lint

a62ce2a

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

add test

f598176

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>

noklam requested a review from antonymilne July 21, 2022 16:42

antonymilne approved these changes Jul 21, 2022

View reviewed changes

noklam requested review from AhdraMeraliQB and removed request for SajidAlamQB July 22, 2022 13:51

AhdraMeraliQB approved these changes Jul 22, 2022

View reviewed changes

noklam merged commit 5ae97cf into main Jul 22, 2022

noklam deleted the fix/docs_session_create branch July 22, 2022 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update documentation to show how to use `KedroSession` outside of CLI mode #1713

Update documentation to show how to use `KedroSession` outside of CLI mode #1713

noklam commented Jul 19, 2022 •

edited

Loading

noklam Jul 19, 2022

antonymilne Jul 21, 2022

antonymilne Jul 21, 2022

noklam commented Jul 19, 2022 •

edited

Loading

noklam commented Jul 20, 2022 •

edited

Loading

datajoely commented Jul 20, 2022

noklam commented Jul 20, 2022 •

edited

Loading

datajoely commented Jul 20, 2022

antonymilne Jul 21, 2022

antonymilne Jul 21, 2022

noklam Jul 21, 2022

noklam Jul 21, 2022

noklam Jul 21, 2022

antonymilne Jul 21, 2022

noklam Jul 21, 2022

antonymilne Jul 21, 2022

antonymilne Jul 21, 2022

AhdraMeraliQB left a comment

		@@ -58,7 +58,6 @@ def reload_kedro(

		_remove_cached_modules(metadata.package_name)

		configure_project(metadata.package_name)

Update documentation to show how to use KedroSession outside of CLI mode #1713

Update documentation to show how to use KedroSession outside of CLI mode #1713

Conversation

noklam commented Jul 19, 2022 • edited Loading

Description

Development notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noklam commented Jul 19, 2022 • edited Loading

noklam commented Jul 20, 2022 • edited Loading

datajoely commented Jul 20, 2022

noklam commented Jul 20, 2022 • edited Loading

datajoely commented Jul 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AhdraMeraliQB left a comment

Choose a reason for hiding this comment

Update documentation to show how to use `KedroSession` outside of CLI mode #1713

Update documentation to show how to use `KedroSession` outside of CLI mode #1713

noklam commented Jul 19, 2022 •

edited

Loading

noklam commented Jul 19, 2022 •

edited

Loading

noklam commented Jul 20, 2022 •

edited

Loading

noklam commented Jul 20, 2022 •

edited

Loading