Destroy default project path variable #1955

jmholzer · 2022-10-18T17:42:31Z

Description

Resolves #1840

A summary of the issue is as follows:

The global variable default_project_path of kedro.ipython allows the line magic %reload_kedro to remember the project path. Global variables such as these decrease code quality and maintainability for a number of reasons and should be removed. The project path can either be specified by the user, or an attempt can be made to find it based on the current working directory if it is not specified (using _find_kedro_project). The default_project_path variable is 100% coupled to the 'in-memory' feature of %kedro_reload and is decoupled from the entire remaining code base. To remove it we could do one of two things:

Decouple the 'in-memory' feature from this variable by changing its implementation.
Remove the 'in-memory' feature from %kedro_reload

@AntonyMilneQB gave 3 suggestions of ways to accomplish option 1. @MerelTheisenQB and @AntonyMilneQB asked for user research as a prerequisite to option 2.

Option 1 does not change anything from the user's perspective, it only changes the implementation of the feature in the code base.

This PR implements option 1, because:

The implementation change is relatively straightforward and simplifies maintenance going forward.
Removing the 'in-memory' feature would have an impact on some users.

Development notes

This PR:

Refactors kedro/ipython/__init__.py:
1.1. By delegating the responsibility for resolving a project path to a dedicated function (single responsibility / separation of concerns).
1.2. By sorting the functions in the file in order of importance.
Removes the default_project_path global variable while retaining the 'in-memory' feature of %reload_kedro, which was re-implemented using the 'local scope' functionality provided by IPython.

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: Jannic Holzer <[email protected]>

…dro-org/kedro into destroy_default_project_path_variable Signed-off-by: Jannic Holzer <[email protected]>

Signed-off-by: Jannic Holzer <[email protected]>

…dro-org/kedro into destroy_default_project_path_variable Signed-off-by: Jannic Holzer <[email protected]>

noklam · 2022-10-20T10:00:55Z

kedro/ipython/__init__.py

+    env: str = None,
+    extra_params: Dict[str, Any] = None,
+    local_ns: Optional[Dict[str, Any]] = None,
+) -> None:  # pragma: no cover
    """Function that underlies the %reload_kedro Line magic. This should not be imported
    or run directly but instead invoked through %reload_kedro."""

    # If a path is provided, set it as default for subsequent calls


This is not always true now - if a user is using reload_kedro instead of %reload_kedro. It only remembers the path when it's using the line magic

You're right, removed this comment in 72509ec

noklam · 2022-10-20T10:04:43Z

tests/ipython/test_ipython.py

+        expected = Path("/test").resolve()
+        assert result == expected
+
+    def no_path_no_local_namespace_specified(self, mocker):


should it be a test?

Suggested change

def no_path_no_local_namespace_specified(self, mocker):

def test_no_path_no_local_namespace_specified(self, mocker):

Nice, thanks! Fixed in 211706c

Signed-off-by: Jannic Holzer <[email protected]>

noklam

Looks quite good already as a 1st draft! I left some minor comments there, I think it's nice to keep the memory behaviour.

Removing the global should make the ipython testing easier. Have you manually test the test_ipython.py ? As we skipped these tests it would be great to manually test it.

Signed-off-by: Jannic Holzer <[email protected]>

jmholzer · 2022-10-20T13:31:33Z

Thanks for the review @noklam! 😃 I addressed the comments you left individually.

Removing the global should make the ipython testing easier. Have you manually test the test_ipython.py ? As we skipped these tests it would be great to manually test it.

Yes these tests work, do you know why these are being skipped? Is it due to performance?

Signed-off-by: Jannic Holzer <[email protected]>

merelcht

Generally this looks good 👍 I've got some questions specifically about the local_ns stuff

merelcht · 2022-10-21T15:21:51Z

kedro/ipython/__init__.py

+    path: str = None,
+    env: str = None,
+    extra_params: Dict[str, Any] = None,
+    local_ns: Optional[Dict[str, Any]] = None,


What is local_ns? I'd be in favour of having longer argument names if that means it's more descriptive.

local_ns is the argument that IPython passes to magics that are decorated with needs_local_scope, which is the feature that allows this implementation to work. local_ns is a dict containing all declarations in the scope of the IPython session and is persisted between calls to magics / extensions being reloaded. Unfortunately, the name must be local_ns in the function signature of the magic for IPython to use it correctly. This feature of IPython seems poorly documented, or I would share some docs.

That being said, we should use a more descriptive name elsewhere -- local_namespace. I will also change the project_path key of local_ns to _project_path, to avoid users accidentally overwriting this.

These changes are in 838aaf1

Thanks for explaining @jmholzer, this makes sense now.

kedro/ipython/__init__.py

Signed-off-by: Jannic Holzer <[email protected]>

…dro-org/kedro into destroy_default_project_path_variable Signed-off-by: Jannic Holzer <[email protected]>

Signed-off-by: Jannic Holzer <[email protected]>

jmholzer · 2022-10-21T17:36:13Z

@jmholzer I think we need to manually test out the IPython stuff in GitPod -> try out the kedro ipython command and check if the variables are instantiated properly.

Hey Nok, the PR is in a state where it's ready for some manual testing. I have manually tested the changes and am happy that the functionality is the same as before for the cases I could think of. It would be great if you could test some of the cases you think of too.

Signed-off-by: Jannic Holzer <[email protected]>

SajidAlamQB

This looks good, just had some lingering questions about local_ns and how it exactly works.

SajidAlamQB · 2022-10-24T08:48:45Z

kedro/ipython/__init__.py

+    path: str = None,
+    env: str = None,
+    extra_params: Dict[str, Any] = None,
+    local_namespace: Optional[Dict[str, Any]] = None,


Does this still work, I thought it had to be local_ns or is that just for functions with the decorator, needs_local_scope.

As far as I understood it, it must be local_ns in the magic_reload_kedro method above, but it's okay if it's a different name in the other methods.

merelcht

Nicely done! ⭐
Don't forget to add this change to the release notes 📝

SajidAlamQB

Awesome work on this! Tested it manually all looks good. 🌟

Signed-off-by: Jannic Holzer <[email protected]>

…dro-org/kedro into destroy_default_project_path_variable Signed-off-by: Jannic Holzer <[email protected]>

noklam

Thanks! I manually tested the kedro ipython and it works fine.

kedro ipython
`%reload_kedro # with/without arguments
Check if the context, pipeline, session object are created correctly
session.run works

Signed-off-by: Jannic Holzer <[email protected]>

merelcht · 2022-10-24T16:29:15Z

tests/ipython/test_ipython.py

+        class Context:
+            _project_path = Path("/test").resolve()


Is this class really necessary? I feel like it overcomplicates the test setup a bit.

The context for this change is that I moved to storing the project path in context (an instance of KedroContext), which is a variable in the IPython local namespace. This is the way that Viz does it and it is much safer than storing it in the 'top-level' of the IPython namespace, where a user can accidentally overwrite it.

This means that I had to change the testing implementation, I am using a dummy Context class rather than importing KedroContext and instantiating this, since its __init__ takes a lot of arguments that we do not need here. Do you think I should do the latter instead?

Thanks for explaining! I'm happy to leave it like this, but perhaps add a small comment in the tests with this Context class explaining why it's there. I didn't immediately understand it was the KedroContext and not some completely different context used.

Great, thanks for taking another look :). Made those changes and will merge

Signed-off-by: Jannic Holzer <[email protected]>

…dro-org/kedro into destroy_default_project_path_variable Signed-off-by: Jannic Holzer <[email protected]>

* Refactor function order Signed-off-by: Jannic Holzer <[email protected]> * Add implementation for _resolve_and_update_project_path Signed-off-by: Jannic Holzer <[email protected]> * Remove fixture for default_project_path Signed-off-by: Jannic Holzer <[email protected]> * Move check for unresolvable project name to extension load Signed-off-by: Jannic Holzer <[email protected]> * Remove 'raises TypeError' from docstring Signed-off-by: Jannic Holzer <[email protected]> * Add test coverage for _resolve_project_path Signed-off-by: Jannic Holzer <[email protected]> * Fix linting :/ Signed-off-by: Jannic Holzer <[email protected]> * Add cross-platform support for tests Signed-off-by: Jannic Holzer <[email protected]> * More cross-platform path changes Signed-off-by: Jannic Holzer <[email protected]> * Linting :s Signed-off-by: Jannic Holzer <[email protected]> * Remove comment regarding path update Signed-off-by: Jannic Holzer <[email protected]> * Add missing test_* prefix Signed-off-by: Jannic Holzer <[email protected]> * Fix path for test on Windows Signed-off-by: Jannic Holzer <[email protected]> * Add mocker patch to fix failing test Signed-off-by: Jannic Holzer <[email protected]> * Lint with black Signed-off-by: Jannic Holzer <[email protected]> * Move unresovable path check to load_ipython_extension Signed-off-by: Jannic Holzer <[email protected]> * Modify call to magic('reload_kedro') to simple call to reload_kedro Signed-off-by: Jannic Holzer <[email protected]> * Add tests for the case with an unresolvable project Signed-off-by: Jannic Holzer <[email protected]> * Lint Signed-off-by: Jannic Holzer <[email protected]> * Modify local_ns to local_namespace in _resolve_project_path Signed-off-by: Jannic Holzer <[email protected]> * Add test for updating _project_path in local_ns Signed-off-by: Jannic Holzer <[email protected]> * Remove unused pylint ignores Signed-off-by: Jannic Holzer <[email protected]> * Add description of change Signed-off-by: Jannic Holzer <[email protected]> * Store _project_path in 'context' if available Signed-off-by: Jannic Holzer <[email protected]> * Disable pylint warnings Signed-off-by: Jannic Holzer <[email protected]> * Rename Context() to DummyKedroContext() to make intention clearer Signed-off-by: Jannic Holzer <[email protected]> * Add comment explaining purpose of DummyKedroContext class Signed-off-by: Jannic Holzer <[email protected]> * Rename DummyKedroContext to MockKedroContext Signed-off-by: Jannic Holzer <[email protected]> Signed-off-by: Jannic Holzer <[email protected]> Signed-off-by: nickolasrm <[email protected]>

jmholzer and others added 15 commits October 18, 2022 16:29

Refactor function order

58cf19a

Signed-off-by: Jannic Holzer <[email protected]>

Add implementation for _resolve_and_update_project_path

29d75de

Signed-off-by: Jannic Holzer <[email protected]>

Remove fixture for default_project_path

af27b01

Signed-off-by: Jannic Holzer <[email protected]>

Move check for unresolvable project name to extension load

47c7316

Signed-off-by: Jannic Holzer <[email protected]>

Remove 'raises TypeError' from docstring

674ca4f

Signed-off-by: Jannic Holzer <[email protected]>

Add test coverage for _resolve_project_path

33fef32

Signed-off-by: Jannic Holzer <[email protected]>

Fix linting :/

7ad2fdf

Signed-off-by: Jannic Holzer <[email protected]>

Merge branch 'main' into destroy_default_project_path_variable

632d106

Merge branch 'main' into destroy_default_project_path_variable

e959ff3

Add cross-platform support for tests

15e9185

Signed-off-by: Jannic Holzer <[email protected]>

Merge branch 'destroy_default_project_path_variable' of github.com:ke…

3301fdb

…dro-org/kedro into destroy_default_project_path_variable Signed-off-by: Jannic Holzer <[email protected]>

More cross-platform path changes

7d28e32

Signed-off-by: Jannic Holzer <[email protected]>

Merge branch 'main' into destroy_default_project_path_variable

efd559b

Linting :s

275b925

Signed-off-by: Jannic Holzer <[email protected]>

Merge branch 'destroy_default_project_path_variable' of github.com:ke…

2d8be68

…dro-org/kedro into destroy_default_project_path_variable Signed-off-by: Jannic Holzer <[email protected]>

jmholzer requested review from merelcht, antonymilne and noklam October 19, 2022 17:52

jmholzer marked this pull request as ready for review October 19, 2022 18:20

jmholzer requested a review from idanov as a code owner October 19, 2022 18:20

jmholzer requested a review from SajidAlamQB October 20, 2022 09:03

noklam reviewed Oct 20, 2022

View reviewed changes

Remove comment regarding path update

72509ec

Signed-off-by: Jannic Holzer <[email protected]>

noklam reviewed Oct 20, 2022

View reviewed changes

jmholzer added 4 commits October 20, 2022 11:10

Add missing test_* prefix

211706c

Signed-off-by: Jannic Holzer <[email protected]>

Fix path for test on Windows

1f0aded

Signed-off-by: Jannic Holzer <[email protected]>

Add mocker patch to fix failing test

5d32316

Signed-off-by: Jannic Holzer <[email protected]>

Lint with black

ff8df19

Signed-off-by: Jannic Holzer <[email protected]>

Add tests for the case with an unresolvable project

cdb327b

Signed-off-by: Jannic Holzer <[email protected]>

merelcht reviewed Oct 21, 2022

View reviewed changes

jmholzer and others added 5 commits October 21, 2022 16:32

Lint

387bc53

Signed-off-by: Jannic Holzer <[email protected]>

Merge branch 'main' into destroy_default_project_path_variable

90c8fd3

Modify local_ns to local_namespace in _resolve_project_path

838aaf1

Signed-off-by: Jannic Holzer <[email protected]>

Merge branch 'destroy_default_project_path_variable' of github.com:ke…

542e697

…dro-org/kedro into destroy_default_project_path_variable Signed-off-by: Jannic Holzer <[email protected]>

Add test for updating _project_path in local_ns

783f6c3

Signed-off-by: Jannic Holzer <[email protected]>

Remove unused pylint ignores

494ce38

Signed-off-by: Jannic Holzer <[email protected]>

SajidAlamQB reviewed Oct 24, 2022

View reviewed changes

merelcht approved these changes Oct 24, 2022

View reviewed changes

Merge branch 'main' into destroy_default_project_path_variable

446631b

SajidAlamQB approved these changes Oct 24, 2022

View reviewed changes

jmholzer added 2 commits October 24, 2022 10:52

Add description of change

7016a08

Signed-off-by: Jannic Holzer <[email protected]>

Merge branch 'destroy_default_project_path_variable' of github.com:ke…

8813e7e

…dro-org/kedro into destroy_default_project_path_variable Signed-off-by: Jannic Holzer <[email protected]>

noklam self-requested a review October 24, 2022 10:24

noklam approved these changes Oct 24, 2022

View reviewed changes

jmholzer added 2 commits October 24, 2022 17:02

Store _project_path in 'context' if available

6da7bfd

Signed-off-by: Jannic Holzer <[email protected]>

Disable pylint warnings

168221b

Signed-off-by: Jannic Holzer <[email protected]>

merelcht reviewed Oct 24, 2022

View reviewed changes

jmholzer and others added 5 commits October 25, 2022 18:26

Rename Context() to DummyKedroContext() to make intention clearer

11a4485

Signed-off-by: Jannic Holzer <[email protected]>

Add comment explaining purpose of DummyKedroContext class

cf7ceec

Signed-off-by: Jannic Holzer <[email protected]>

Merge branch 'main' into destroy_default_project_path_variable

427aca9

Rename DummyKedroContext to MockKedroContext

4d4c607

Signed-off-by: Jannic Holzer <[email protected]>

Merge branch 'destroy_default_project_path_variable' of github.com:ke…

6548342

…dro-org/kedro into destroy_default_project_path_variable Signed-off-by: Jannic Holzer <[email protected]>

jmholzer merged commit f49234c into main Oct 25, 2022

jmholzer deleted the destroy_default_project_path_variable branch October 25, 2022 18:59

jmholzer mentioned this pull request Dec 2, 2022

load_ipython_extension does not register_magic_function "reload_kedro" #2060

Closed

AhdraMeraliQB mentioned this pull request Dec 23, 2022

Define user journeys for manual testing #2160

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destroy default project path variable #1955

Destroy default project path variable #1955

jmholzer commented Oct 18, 2022 •

edited

Loading

noklam Oct 20, 2022

jmholzer Oct 20, 2022 •

edited

Loading

noklam Oct 20, 2022

jmholzer Oct 20, 2022

noklam left a comment

jmholzer commented Oct 20, 2022 •

edited

Loading

merelcht left a comment

merelcht Oct 21, 2022

jmholzer Oct 21, 2022

merelcht Oct 24, 2022

jmholzer commented Oct 21, 2022

SajidAlamQB left a comment

SajidAlamQB Oct 24, 2022

merelcht Oct 24, 2022

merelcht left a comment

SajidAlamQB left a comment

noklam left a comment

merelcht Oct 24, 2022

jmholzer Oct 24, 2022

merelcht Oct 25, 2022

jmholzer Oct 25, 2022

	def no_path_no_local_namespace_specified(self, mocker):
	def test_no_path_no_local_namespace_specified(self, mocker):

Destroy default project path variable #1955

Destroy default project path variable #1955

Conversation

jmholzer commented Oct 18, 2022 • edited Loading

Description

Development notes

Checklist

Choose a reason for hiding this comment

jmholzer Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noklam left a comment

Choose a reason for hiding this comment

jmholzer commented Oct 20, 2022 • edited Loading

merelcht left a 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

jmholzer commented Oct 21, 2022

SajidAlamQB left a comment

Choose a reason for hiding this 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

SajidAlamQB left a comment

Choose a reason for hiding this comment

noklam left a 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

jmholzer commented Oct 18, 2022 •

edited

Loading

jmholzer Oct 20, 2022 •

edited

Loading

jmholzer commented Oct 20, 2022 •

edited

Loading