Add new function to specify namespace for `KedroSession` and `kedro run` #2306

noklam · 2023-02-10T07:13:53Z

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Close #2145

Description

Currently there's a way to select different sections from a pipeline to run, but selecting by namespace is missing. We should add a way for users to run a specific subpipeline (namespace) by executing this in the terminal:

kedro run --namespace data_engineering.preprocessing

kedro run -ns data_engineering

Development notes

Update session.py to add namespace as record data and modify the test accordingly
Update project.py to add new option for the -ns or --namespace

How this is tested?

It is tested manually with a modified version of spaceflight, since the current starters do not use namespace, I followed this modular pipeline documentation for testing.

Steps

kedro new -s spaceflights
Follow along the page to modify it as a namesapce pipeline (https://kedro.readthedocs.io/en/stable/tutorial/add_another_pipeline.html#optional-modular-pipelines)

Kedro CLI

kedro run --namespace active_modelling - 3 nodes should be run
kedro run --namespace active_modelling --pipeline data_science - 3 3 nodes should be run, since the namespace only available in one pipeline
kedro run --namespace 1234 -- Error ValueError: Pipeline does not contain nodes with namespace '1234'

KedroSession

Similar tests are run with the kedro ipython. run %reload_kedro to refresh a session

session.run(namespace="active_modelling")
session.run(namespace="active_modelling", pipeline_name="data_science")
session.run(namespace="1234") -- error ValueError: Pipeline does not contain nodes with namespace '1234'

Questions for Reviewers

Do we need a specific test for the namespace option? I don't think it's needed as it is more or less equal to other arguments and the filtering function is tested in the test_pipeline.py already.
The ordering for these cli options and KedroSession.run is kind of arbitrary, for KedroSession it is the code API so it makes sense to add the new option as the last argument to keep it backward compatible. Noted some arguments have inconsistent names. For example kedro run --pipeline vs KedroSession.run(pipline_name=xxx)

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>

datajoely · 2023-02-13T11:10:28Z

Super cool!

jmholzer

Nice work ✅

Do we need a specific test for the namespace option? I don't think it's needed as it is more or less equal to other arguments and the filtering function is tested in the test_pipeline.py already.

I think the updated test_run_with_pipeline_filters makes an extra test unnecessary.

The ordering for these cli options and KedroSession.run is kind of arbitrary, for KedroSession it is the code API so it makes sense to add the new option as the last argument to keep it backward compatible.

I think the ordering of click.option decorators is consistent with the order in which their passed value appears in the function signature of run, so personally I think it should be added to the end if possible.

Noted some arguments have inconsistent names. For example kedro run --pipeline vs KedroSession.run(pipline_name=xxx)

I'll take a look if this is on our radar, if not I'll open a ticket.

jmholzer · 2023-02-13T11:31:07Z

kedro/framework/session/session.py

@@ -336,6 +337,7 @@ def run(  # pylint: disable=too-many-arguments,too-many-locals
                used as an end point of the new ``Pipeline``.
            load_versions: An optional flag to specify a particular dataset
                version timestamp to load.
+            namespace: The namespace of the nodes that is being run.


Suggested change

namespace: The namespace of the nodes that is being run.

namespace: An optional string specifying a namespace to run. If specified, only the nodes in this namespace will be run.

Nice! @jmholzer

jmholzer · 2023-02-13T11:40:19Z

kedro/framework/cli/project.py

@@ -353,6 +355,7 @@ def activate_nbstripout(
    callback=_reformat_load_versions,
 )
 @click.option("--pipeline", "-p", type=str, default=None, help=PIPELINE_ARG_HELP)
+@click.option("--namespace", "-ns", type=str, default=None, help=NAMESPACE_ARG_HELP)


To keep the ordering of click.option decorators consistent with the arguments in the function signature, I think this should be moved to the bottom.

Do we actually follow the order? The first click option is from_input but the first argument would be tag. I think it only affects when you do kedro run -h, it will follow the order that how you decorate your function. In general, I think it should follow the order of importance. I would expect pipeline to be the first argument in that case.

I don't have a strong opinion about moving it to the back, the only reason I keep it next to pipeline is that I feel these two arguments are in the same category. Similarly, the nodes arguments are together, and the config-related arguments are grouped as well. @merelcht or @AntonyMilneQB Is this something we've discussed in the past?

Fully agree with you here @noklam 👍 We should go for what makes sense to the user when you do kedro run --help; no need to be consistent with the order of KedroSession.run (which as you've said is somewhat arbitrary).

antonymilne

LGTM ⭐

On the questions you ask:

Agreed with @jmholzer, tests look sufficient 👍
Agreed with @noklam. We should aim for a good CLI UX first and then consistency with Python API second. Adding the argument to the end of the KedroSession.run is correct to maintain backwards compatibility but doesn't need to be reflected in the click run command - as you notice, we already don't have consistency there. (Backwards compatibility is kind of an edge case here because I doubt anyone calls KedroSession.run with positional arguments, but technically what you've done is right.)

Going beyond this PR, it would be nice to have eventual consistency between the Python API and click run command. More importantly than the order is the naming of the arguments. This is something I've noticed before and it's on our radar here: #2247. Note the pipeline_name vs. pipeline question follows the same pattern as the node_name vs. node one noted there.

In fact, I now have another thought on the ordering of arguments - will add it to that issue to keep it in one place.

RELEASE.md

antonymilne · 2023-02-14T06:37:37Z

kedro/framework/cli/project.py

+NAMESPACE_ARG_HELP = """Name of the namespace to run.
+If not set, no namespace is used."""


Suggested change

NAMESPACE_ARG_HELP = """Name of the namespace to run.

If not set, no namespace is used."""

NAMESPACE_ARG_HELP = """Name of the node namespace to run."""

IMO the "no namespace is used" just makes this less clear since it can't be really explained fully in just a line.

antonymilne · 2023-02-14T06:39:04Z

kedro/framework/cli/project.py

@@ -353,6 +355,7 @@ def activate_nbstripout(
    callback=_reformat_load_versions,
 )
 @click.option("--pipeline", "-p", type=str, default=None, help=PIPELINE_ARG_HELP)
+@click.option("--namespace", "-ns", type=str, default=None, help=NAMESPACE_ARG_HELP)


Fully agree with you here @noklam 👍 We should go for what makes sense to the user when you do kedro run --help; no need to be consistent with the order of KedroSession.run (which as you've said is somewhat arbitrary).

antonymilne · 2023-02-14T07:16:13Z

@noklam One thing I just thought of. We should add this to the documentation here. Maybe other places too, have a look. But that's the one that springs to mind.

noklam · 2023-02-14T07:19:01Z

Thank you @AntonyMilneQB, I will update the docs🙂

Signed-off-by: Nok <nok_lam_chan@mckinsey.com>

AhdraMeraliQB · 2023-02-17T11:01:34Z

tests/framework/cli/test_cli.py

@@ -562,9 +565,13 @@ def test_run_with_pipeline_filters(
    ):
        from_nodes = ["--from-nodes", "splitting_data"]
        to_nodes = ["--to-nodes", "training_model"]
+        tags = ["--tag", "de"]
+        namespace = ["--namespace", "fake_namespace"]
        tags = ["--tags", "de"]


Looks like tags is being overwritten here 🤔

AhdraMeraliQB · 2023-02-17T11:03:06Z

kedro/framework/session/session.py

@@ -336,6 +337,7 @@ def run(  # pylint: disable=too-many-arguments,too-many-locals
                used as an end point of the new ``Pipeline``.
            load_versions: An optional flag to specify a particular dataset
                version timestamp to load.
+            namespace: The namespace of the nodes that is being run.


Nice! @jmholzer

Signed-off-by: Nok <nok_lam_chan@mckinsey.com>

…un` (#2306) * Add new namespace options for KedroSession and `kedro run` Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com> * Change the shortcut from `--ns` to `-ns` to be consistent Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com> * Fix lint Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com> * Fix broken tests and update new test for namespace argument Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com> * update docs and release notes Signed-off-by: Nok <nok_lam_chan@mckinsey.com> * Fix tests Signed-off-by: Nok <nok_lam_chan@mckinsey.com> * Fix test for incorrect merge Signed-off-by: Nok <nok_lam_chan@mckinsey.com> --------- Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com> Signed-off-by: Nok <nok_lam_chan@mckinsey.com> Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>

noklam added 2 commits February 10, 2023 12:51

Merge branch 'main' of github.com:kedro-org/kedro

4565684

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

Add new namespace options for KedroSession and kedro run

f3f274c

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

noklam requested a review from idanov as a code owner February 10, 2023 07:13

Change the shortcut from --ns to -ns to be consistent

c46d19b

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

noklam marked this pull request as draft February 10, 2023 07:24

noklam added 2 commits February 13, 2023 13:15

Fix lint

3eed222

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

Fix broken tests and update new test for namespace argument

f54f95d

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

noklam marked this pull request as ready for review February 13, 2023 09:49

noklam requested review from merelcht, jmholzer and AhdraMeraliQB and removed request for merelcht February 13, 2023 09:49

jmholzer approved these changes Feb 13, 2023

View reviewed changes

antonymilne reviewed Feb 14, 2023

View reviewed changes

antonymilne approved these changes Feb 14, 2023

View reviewed changes

antonymilne mentioned this pull request Feb 14, 2023

Investigate further inconsistencies between the CLI and Kedro API #2247

Closed

update docs and release notes

6e5c4cc

Signed-off-by: Nok <nok_lam_chan@mckinsey.com>

noklam requested a review from yetudada as a code owner February 16, 2023 07:54

Merge branch 'main' into feat/add-namespace-option-kedro-run

e8a386a

noklam removed request for idanov and yetudada February 16, 2023 07:55

noklam and others added 3 commits February 16, 2023 17:57

Merge branch 'main' into feat/add-namespace-option-kedro-run

afed303

Merge branch 'main' into feat/add-namespace-option-kedro-run

6c75c80

Fix tests

b179822

Signed-off-by: Nok <nok_lam_chan@mckinsey.com>

AhdraMeraliQB approved these changes Feb 17, 2023

View reviewed changes

noklam and others added 2 commits February 17, 2023 19:08

Merge branch 'main' into feat/add-namespace-option-kedro-run

7f83809

Fix test for incorrect merge

2b6c087

Signed-off-by: Nok <nok_lam_chan@mckinsey.com>

Merge branch 'main' into feat/add-namespace-option-kedro-run

a1acb12

noklam merged commit 116ddd0 into main Feb 20, 2023

noklam deleted the feat/add-namespace-option-kedro-run branch February 20, 2023 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new function to specify namespace for `KedroSession` and `kedro run` #2306

Add new function to specify namespace for `KedroSession` and `kedro run` #2306

noklam commented Feb 10, 2023 •

edited

Loading

datajoely commented Feb 13, 2023

jmholzer left a comment

jmholzer Feb 13, 2023

AhdraMeraliQB Feb 17, 2023 •

edited

Loading

jmholzer Feb 13, 2023

noklam Feb 13, 2023

antonymilne Feb 14, 2023

antonymilne left a comment •

edited

Loading

antonymilne Feb 14, 2023

antonymilne Feb 14, 2023

antonymilne commented Feb 14, 2023

noklam commented Feb 14, 2023

AhdraMeraliQB Feb 17, 2023

AhdraMeraliQB Feb 17, 2023 •

edited

Loading

	namespace: The namespace of the nodes that is being run.
	namespace: An optional string specifying a namespace to run. If specified, only the nodes in this namespace will be run.

		NAMESPACE_ARG_HELP = """Name of the namespace to run.
		If not set, no namespace is used."""

	NAMESPACE_ARG_HELP = """Name of the namespace to run.
	If not set, no namespace is used."""
	NAMESPACE_ARG_HELP = """Name of the node namespace to run."""

Add new function to specify namespace for KedroSession and kedro run #2306

Add new function to specify namespace for KedroSession and kedro run #2306

Conversation

noklam commented Feb 10, 2023 • edited Loading

Description

Description

Development notes

How this is tested?

Kedro CLI

KedroSession

Questions for Reviewers

Checklist

datajoely commented Feb 13, 2023

jmholzer left a comment

Choose a reason for hiding this comment

jmholzer Feb 13, 2023

Choose a reason for hiding this comment

AhdraMeraliQB Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

jmholzer Feb 13, 2023

Choose a reason for hiding this comment

noklam Feb 13, 2023

Choose a reason for hiding this comment

antonymilne Feb 14, 2023

Choose a reason for hiding this comment

antonymilne left a comment • edited Loading

Choose a reason for hiding this comment

antonymilne Feb 14, 2023

Choose a reason for hiding this comment

antonymilne Feb 14, 2023

Choose a reason for hiding this comment

antonymilne commented Feb 14, 2023

noklam commented Feb 14, 2023

AhdraMeraliQB Feb 17, 2023

Choose a reason for hiding this comment

AhdraMeraliQB Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Add new function to specify namespace for `KedroSession` and `kedro run` #2306

Add new function to specify namespace for `KedroSession` and `kedro run` #2306

noklam commented Feb 10, 2023 •

edited

Loading

AhdraMeraliQB Feb 17, 2023 •

edited

Loading

antonymilne left a comment •

edited

Loading

AhdraMeraliQB Feb 17, 2023 •

edited

Loading