kedro-org / kedro

Kedro is a toolbox for production-ready data science. It uses software engineering best practices to help you create data engineering and data science pipelines that are reproducible, maintainable, and modular.
https://kedro.org
Apache License 2.0
9.89k stars 897 forks source link

Proposal for Partial/Custom node ordering for SequentialRunner #3717

Closed noklam closed 6 months ago

noklam commented 6 months ago

Description

Support custom / partial node order where user desired.

Background

Kedro offers 3 Runners out of the box (SequentialRunner, ParallelRunner and ThreadRunner), there is another SoftFailRunner which I have implemented and can be installed in https://pypi.org/project/kedro-softfail-runner/.

In the past we have focus to make consistent support for runners and are reluctant to introduce feature parity. In fact, runners has been mostly unchanged for years. Improve resume pipeline suggestion for SequentialRunner introduce a concept similar to "Change Data Capture" (CDC), which fixed the broken suggestion and started to consider about persisted data and the closest checkpoint to recover a failed pipeline. This is the most obvious feature parity among runners as it only support SequentialRunner due to the non-deterministic nature of parallel computing.

This proposal will only support SequentialRunner, which I am increasingly more comfortable with, details are discussed in #3713.

Design

Toposort will give ONE feasible solution, while there can be multiple possible solutions. In some case:

Requirements:

Nice to have:

Feature:

Next step is providing an user friendly API, as this is likely still too low-level for the end user.

High level Proposal

Add new constrains to "node_dependencies" addition to the existing inputs/outputs pair.

Possible Implementation

Can't think of anything, thus dummy outputs has been the workaround for years.

Possible Alternatives

Current workaround involves dummy inputs outputs which become tedious quickly.

datajoely commented 6 months ago

How would you imagine this work?

noklam commented 6 months ago

feature branch: @datajoely https://github.com/kedro-org/kedro/compare/main...noklam/node-ordering-proposal

Don't focus on the API, it's hacky and there is many random Pipeline call in the current process so I have to patch that everywhere.

Demo Project: kedro-partial-node-order

comment or uncomment this line to play with it.

https://github.com/noklam/kedro-partial-node-order/blob/75b2febb45ab3a44f93928d9d0796bb9d9765ef7/src/ls/pipelines/data_processing/pipeline.py#L40

There are many way to build a nicer API, for example:

It's not too important to decide this now, it's more of a PoC to prove this is possible. Benefit of this is Kedro viz won't see this at all (I think)

We can also assume it always follow the order of declaration, or try to stick with it as much as possible. Those are just design decision so I will delay that discussion.

datajoely commented 6 months ago

Okay so I understand what you're trying to do. But have some questions.

  1. Why is asking users to pass dummy datasets between nodes insufficient?
  2. Is a deterministic sort (something like a seed) order a more useful enhancement?
  3. This feels like something that should happen at a orchestor / namespace granularity level first, so would address some of the recommendations in #3094 first.
noklam commented 6 months ago
  1. Why is asking users to pass dummy datasets between nodes insufficient? Few reasons:

    • It contaminate the DAG flowchart (kedro-viz)
    • It's hard to edit or read custom execution order, users will need to go through nodes and try to do that matching in their head or edit multiple file.
    • It feels very hacky.
    • It's annoying that you cannot make Kedro run sequentially (or at least follow the declaration order when it's possible)
  2. Is a deterministic sort (something like a seed) order a more useful enhancement?

If you run with SequenetialRunner now, it's deterministic already https://github.com/kedro-org/kedro/pull/1604. Not sure if this is what you are talking about.

  1. This feels like something that should happen at a orchestor / namespace granularity level first, so would address some of the recommendations in #3094 first.

Which recommendation are you referring to? there are many mentioned.

datajoely commented 6 months ago
  1. You've sold me
  2. Wasn't aware this was fixed, so yeah your prioritization is right.
  3. I think the user provided session ID is the only one I'd put ahead of this.
noklam commented 6 months ago

@datajoely 2. is one the thing that annoys me before I joined thus the first thing I fixed :P (also requested by former colleague), CacheDataset is the other one that I still haven't fixed.

datajoely commented 6 months ago

We'll get there 🚀