Merge "modular pipeline" functionality into Pipeline class

[merelcht]

Proposal: Unifying modular_pipelines.py and pipeline.py

Summary

This proposal suggests merging the functionality of modular_pipelines.py into pipeline.py to align with the design pattern used in nodes.py. In nodes.py, the Node class and the node() function coexist, where node() simply creates an instance of Node.

Proposed Changes

• Move the pipeline() function into pipeline.py, ensuring it instantiates a Pipeline object directly.
• Update Pipeline to accept the same arguments as pipeline(), including namespace, inputs, outputs, and parameters.
• Introduce _map_nodes() as a helper function to handle namespace mapping for inputs, outputs, and parameters, maintaining clarity and separation of concerns.

Note

After discussing further, it was decided not to remove the pipeline() and node() functions. While these methods aren't doing much more than instantiating the class objects, removing them will be a completely unnecessary breaking change and something that will affect a lot of users.

Benefits

• Aligns Pipeline with the existing Node design pattern, improving consistency.
• Simplifies the structure by consolidating related functionality into a single file.
• Enhances the pipeline API
• No behaviour loss with this approach

Context

#2805

POC/Spike

#4563
(this is not meant to be merged as is, but to show what the proposal will approximately look like)

Deprecation plan

Non breaking changes on main

1. Copy functionality from modular_pipelines.py to pipeline.py
2. Add additional arguments to Pipeline class
3. Deprecate the modular_pipelines module and it's methods

Breaking changes on develop

1. Remove modular_pipelines.py
2. Change argument names for consistency (e.g. pipe becomes nodes)
3. Rename ModularPipelineError to PipelineError

[astrojuanlu]

One question that is not entirely clear to me, does this plan (after the breaking changes in develop) fully fix #2805 ?

We import pipeline in our starter and this confuse IDE like VSCode, as it thinks pipeline is a module instead of the pipeline function.

from kedro.pipeline import Pipeline, node, pipeline

[astrojuanlu]

My understanding is that, the only user-facing breaking change of this proposal is that, in Kedro 1.0, from kedro.pipeline.modular_pipeline import * would stop working. Am I missing anything?

[merelcht]

I think it will, but this is about much more than only fixing an IDE confusion. The fact that we are being "clever" and having this hack so you can do from kedro.pipeline import pipeline and it will import it from modular_pipeline is really confusing. It's also inconsistent with the Node node architecture and never meant to be the final design. The team just didn't have time to address this tech debt before.

[astrojuanlu]

Understood. Since this is then mostly an internal refactoring effort with little user impact, I have nothing else to say. Left 1 comment on the draft PR about a breaking change.

[rashidakanchwala]

we can also discussed redirecting the kedro.pipeline.modular_pipeline import right? Are we still gonna do that? because then it woudn't break anything for the user.

[merelcht]

@rashidakanchwala for 1.0.0 I'd still suggest removing even a redirect, so that would just be a different approach to the non-breaking changes I proposed.

[ankatiyar]

Agree with the proposed changes :)

[DimedS]

I agree with the changes. I just want to ask a clarification question: am I right in thinking that the pipeline() function will become empty - simply returning the Pipeline class with the same set of arguments? And that all the logic currently in pipeline() will be moved inside the class?

[merelcht]

Yes @DimedS, you can see it in the POC draft PR, but essentially it will become:

def pipeline(  # noqa: PLR0913
    pipe: Iterable[Node | Pipeline] | Pipeline,
    *,
    inputs: str | set[str] | dict[str, str] | None = None,
    outputs: str | set[str] | dict[str, str] | None = None,
    parameters: str | set[str] | dict[str, str] | None = None,
    tags: str | Iterable[str] | None = None,
    namespace: str | None = None,
) -> Pipeline:
   

    return Pipeline(
        nodes=pipe,
        inputs=inputs,
        outputs=outputs,
        parameters=parameters,
        tags=tags,
        namespace=namespace,
    )

[DimedS]

thanks, looks great!

[rashidakanchwala]

agree with the changes!

[astrojuanlu]

Agreed!

[deepyaman]

In general, agreed that it would be good to remove the longer modular pipeline import and consolidate the functionality. However, I have a few thoughts and/or questions:

1. Change argument names for consistency (e.g. pipe becomes nodes)

   Is this most accurate? I think it's fine, looking at the existing implementation, but should make sure to note that a pipeline can be passed here and is just treated as a collection of nodes.

2. from kedro.pipeline import Pipeline, node, pipeline

   Can we avoid importing both Pipeline and pipeline? In this world, you still need both if you're type checking:

   def create_pipeline(**kwargs) -> Pipeline:
       return pipeline(
           [
               node(
                   func=split_data,
                   inputs=["model_input_table", "params:model_options"],
                   outputs=["X_train", "X_test", "y_train", "y_test"],
                   name="split_data_node",
               ),
               node(
                   func=train_model,
                   inputs=["X_train", "y_train"],
                   outputs="regressor",
                   name="train_model_node",
               ),
               node(
                   func=evaluate_model,
                   inputs=["regressor", "X_test", "y_test"],
                   outputs=None,
                   name="evaluate_model_node",
               ),
           ],
           parameters={"params:model_options": "params:model_options_1"},
       )

   It seems like the pipeline function is completely unnecessary.

   After discussing further, it was decided not to remove the pipeline() and node() functions. While these methods aren't doing much more than instantiating the class objects, removing them will be a completely unnecessary breaking change and something that will affect a lot of users.

   At the same time, I understand we don't want to break existing code, so I'd propose keeping the pipeline function but transitioning all examples and docs over the use Pipeline. I think it's also possible to throw some sort of deprecation, could even be at 1.0, for people still using pipeline? No need to remove it soon, but could nudge people towards the desired pattern without breaking anything.

   Is there any real benefit to keeping the lowercase pipeline beyond backward compatibility? Do people prefer writing a lowercase pipeline? I would think you may as well be explicit that you're just constructing a class, rather than having a function that people have to look up and see produces a class.

   And, if go this route, it would make sense to just recommend use of Node, too?

[astrojuanlu]

(and starters)

[merelcht]

I feel like it’s pretty Pythonic to have lowercase “constructors” for core classes like dict, list, defaultdict, str, etc. It’s seems to be a common pattern for the staple classes that are important for your codebase, and I’d say node and pipeline fall into that category when it comes to Kedro projects. So keeping them lowercase kind of emphasises how essential they are.

[astrojuanlu]

Indeed, although the advantage of the native classes is that the type and the constructor are now the same (from 3.9 onwards by virtue of PEP 585), so that users can write

def foo() -> dict[str, str]:
    return dict({"a": "b"})  # Unnecessary `dict` but written here for clarity

Nobody else has that luxury, and even foundational concepts such as dataframes respect the PEP 8 convention:

import pandas as pd

def foo() -> pd.DataFrame:
    return pd.DataFrame(...)

So far, users had to do from kedro.pipeline import Pipeline, pipeline because there was no other choice, but I'm with @deepyaman in that, even though we keep the pipeline convenience function to avoid breaking everyone's code, our starters and docs would benefit from doing

from kedro.pipeline import Pipeline

def create_pipeline() -> Pipeline:
    return Pipeline(...)

The alternatives are (a) the current status (less than ideal, and unnecessary after the changes in this proposal) or (b) even weirder,

from kedro.pipeline import pipeline

def create_pipeline() -> pipeline:  # !!!
    def pipeline(...)

which goes against every other Python library I've used...

[Galileo-Galilei]

Definitely agree with the changes, and with all comments in this thread

[astrojuanlu]

This is now awaiting for TSC approval at #4675

[idanov]

I don't think this is a good idea for multiple reasons:

• all existing code uses lowercase
• all existing learning resources use lowercase
• the lowercase is visually nicer to the eye and easier to type
• all LLM is trained on lowercase
• given all of the above, change in style will only introduce confusion for new users and LLM suggestions too
• experience with previous useless renamings (i.e. the DataSet to Dataset) shows that there's zero benefits and lots of frustration with users (mentioned in multiple of interviews that the renaming of DataSet to Dataset caused a lot of frustration with users)
• in Python, essential and staple constructors like dict, list, tuple are all lowercase, which the node and pipeline emulate by showing how essential and staple constructors they are for a Kedro project
• node can be very easily reworked into a decorator, thus keeping the naming case between declarative vs decorated

Before we go with this suggestion, can we get a few valid reasons why this is necessary? In general it will be a good practice to always discuss breaking changes (or preludes to breaking changes), especially major ones like this one, to be reasoned from the principle: It's a no, until you prove that yes is worthwhile. At this point, after nearly 7 years of history using lowercase, I think it's a bit too late to vote entirely based on preference (as none of the benefits listed are real benefits or explained concretely enough, so the only reason I see for this change is personal preferences).

PS: My argument mainly focuses on Node, while I acknowledge that for Pipeline you could see much less history or support for suggesting lowercase pipeline instead of Pipeline. While my no in the vote stays for any suggestions for node, I think I could be convinced that Pipeline should be the recommended version.

[astrojuanlu]

The argument for Pipeline vs pipeline is explained here #2805 long history of IDEs getting confused, several users commenting there. Further thoughts outlined a bit above this thread #4594 (reply in thread)

The argument for Node vs node is a consistency one. Given that code will actually continue working, at least for now, I don't think this is equivalent to the DataSet renaming. That experience does inform how should we approach this though, to avoid repeating the same mistakes.

[merelcht]

Closing this discussion as the changes have been implemented and released in 1.0.0.