Improve resume suggestions

[ondrejzacha]

Description

Supersedes #3026

Addresses #3002:

• Parameter inputs are considered persistent, therefore a failed node with a parameter input won't necessarily trigger re-run of their ancestor nodes.

Additionally:

• Only nodes producing non-persistent inputs are suggested for rerun (as opposed to all parent nodes of a node with 1+ non-persistent inputs)
• Number of suggested nodes to re-run from is minimised (-> shorter message for the same pipeline)
• Refactored into a testable function (therefore more applicable to other runners too)

Development notes

Additional tests added in test_sequential_runner.py – both to check the message in logs for sequential runner specifically, as well as a new suite of tests for the suggestion logic itself. I appreciate that the logic is not exactly straightforward, so please double check the tests / let me know if more tests are needed.

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

• Read the contributing guidelines
• Signed off each commit with a Developer Certificate of Origin (DCO)
• 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
• Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

[noklam]

The function name is _node_with_external_inputs but its arguments is a list of node. If you are trying to search for pipeline node that has external_inputs, can you use pipeline.inputs and then pipeline.only_nodes_with_inputs?

[ondrejzacha]

Good point, simplified that

[noklam]

Suggested change

	all_nodes_that_need_to_run = find_all_required_nodes(
	nodes_to_be_run = find_all_required_nodes(

?

[noklam]

Is this needed for "Number of suggested nodes to re-run from is minimised (-> shorter message for the same pipeline)" this?

[noklam]

I find this a bit hard to understand the purpose of this, is it similar to this?

p = pipeline(all_nodes_tht_need_to_run)
p_inputs = p.inputs()
input_nodes = p.only_nodes_with_input(p_inputs)
input_nodes_names = [n.name for n in input_nodes]

It's just pseudocode so I don't guarantee it works

[ondrejzacha]

This is not exactly the same. The difference appears when nodes share the same external inputs:

Pipeline([
    node(
        name="first_node",
        func=...,
        inputs=["external_input"],
        outputs="intermediate_output",
    ),
    node(
        name="second_node",
        func=...,
        inputs=["external_input", "intermediate_output"],
        outputs="final_output",
    )
])

Your suggestion would produce ["first_node", "second_node"] here, whereas with the topo sort group approach, only the ["first_node"] is produced (as this pipeline will have two topo sort groups, each containing a single node).

[noklam]

I left some comments, and manually tested and it works well.

I have some concerns to open up a lot of public methods which is unclear to me how will it be useful outside of this context (Maybe for using it in notebook? Would love to see some example of this if that's the case)

[ondrejzacha]

I have some concerns to open up a lot of public methods which is unclear to me how will it be useful outside of this context (Maybe for using it in notebook? Would love to see some example of this if that's the case)

Sure, I don't have a strong preference here. In my work I've found myself pulling these methods out of the runner code to figure out how to continue a run of a pipeline which did not fail gracefully (ran into an OOM error and never showed this suggestion) – but I can imagine that can very much be an exception

[merelcht]

Thanks for this improved PR @ondrejzacha ! 🙂 On first review, things look generally good to me. I've left a couple of clarification questions.

The tests look good as well, but I agree with your comment on moving it to a separate test file. The last set of tests aren't actually using any specific runner so it might confuse others coming across them to understand whether this is SequentialRunner specific or not.

[noklam]

Thanks for making all the changes and it is taking so long. This is a great improvement and it's one step closer to rerunning pipeline in an efficient way.

[DimedS]

Thank you, @ondrejzacha , for the significant update to the resume suggestions algorithm and the development of high-quality tests! It's fantastic that the suggestions are now effectively minimised, excluding nodes that have already been successfully processed. I've left some minor comments, but overall, the PR looks great to me!

[merelcht]

Thanks for addressing my comments and this contribution @ondrejzacha ⭐ ⭐

Would you mind updating the release notes with this change as well and adding your name under the contributors section: https://github.com/kedro-org/kedro/blob/main/RELEASE.md#community-contributions ?

[DimedS]

Thank you for addressing comments, @ondrejzacha !

[ondrejzacha]

I think from my side this is ready; the docs build is still failing so let me know if there's anything I can still do. Excited to see this merged, thanks for the reviews! 🤩

[noklam]

@ondrejzacha amazing, thanks again really appreciate the effort🙏 I will take over and merge this.

[DimedS]

Thank you for responding to the comments, @ondrejzacha. Fantastic work on the PR!