Change runner logic to not create pool for sequential runner

[merelcht]

Description

Address #4486

Development notes

Verified that this fixes the issues mentioned in Galileo-Galilei/kedro-mlflow#624 and kedro-org/kedro-plugins#1012

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

[DimedS]

Thanks, @merelcht! That’s a more robust approach. One small question from my side

[ElenaKhaustova]

It looks good to me from a logical point of view.

But I would suggest moving this into SequentialRunner._run. Otherwise, we modify the behaviour of the base class based on what is inherited from it, which is not entirely correct from the implementation point of view and AbstractRunner._run becomes too long. I understand that it will require some duplication, but in the SequentialRunner._run method, we can add a note explaining why we keep the implementation like that. But adding it to AbstractRunner._run will overload it even more.

[merelcht]

I don't have a very strong opinion on this, but my counter argument is then wouldn't it be confusing that the thread and parallel logic is in the AbstractRunner._run method but sequential isn't?

[DimedS]

I agree with Elena that, from a Pythonic perspective, common logic should be placed in _run() within the Abstract class. If there are exceptions, we should override the common logic with specific behavior, which is how runners worked previously. However, I thought the goal of the previous PR, which Merel is currently modifying, was to centralise the runner's logic within the Abstract class.

We had already decided that the _run() function in the abstract class would rely on _get_executor(), which would be implemented specifically in different subclasses. I don't see any issues with this approach. For me, the main question is how large and readable AbstractRunner._run() will be. As Merel pointed out, consolidating all the running logic in one place will be beneficial, which was also the intention of the previous PR.

[merelcht]

I addressed @ElenaKhaustova 's comment about moving the sequential logic to the SequentialRunner in d6a247c . I personally don't have a huge preference for doing it that way or keeping it inside the abstract runner. They both have pros and cons. My main con for having sequential runner override the run method is only that it might be confusing that abstract runner has the logic for thread an parallel execution but not sequential, but arguably it's a bit cleaner.

@DimedS any thoughts?

[astrojuanlu]

Not sure if it's related to what @merelcht is saying, but for the record I was skimming this code and thought "what if pool is None?" (there's no else branch here). Just a comment from the peanut gallery.

[merelcht]

Yeah exactly! I think it might not be immediately obvious that that logic is now inside SequentialRunner.

[DimedS]

I addressed @ElenaKhaustova 's comment about moving the sequential logic to the SequentialRunner in d6a247c . I personally don't have a huge preference for doing it that way or keeping it inside the abstract runner. They both have pros and cons. My main con for having sequential runner override the run method is only that it might be confusing that abstract runner has the logic for thread an parallel execution but not sequential, but arguably it's a bit cleaner.

@DimedS any thoughts?

I agree, I also think that, given the current state, it would be beneficial to keep all the running logic in the Abstract class. I have provided a full comment in the thread: #4502 (comment).