-
Notifications
You must be signed in to change notification settings - Fork 927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change runner logic to not create pool for sequential runner #4502
Open
merelcht
wants to merge
11
commits into
main
Choose a base branch
from
fix/sequential-runner-threads
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
941e132
Change runner logic to not create pool for sequential runner
merelcht e610ce5
Merge branch 'main' into fix/sequential-runner-threads
merelcht d4bd76e
Merge branch 'main' into fix/sequential-runner-threads
merelcht 7b3bd81
Update release notes
merelcht 4c474f4
Merge branch 'main' into fix/sequential-runner-threads
merelcht d6a247c
Move sequential runner logic to sequential runner class
merelcht 89c236b
Fix lint
merelcht d765a72
Pass on get executor for SequentialRunner
merelcht f1390b8
Make _get_executor method not abstract
merelcht 135eb0c
Merge branch 'main' into fix/sequential-runner-threads
merelcht d7a7d03
Revert sequential running logic back to AbstractRunner
merelcht File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 andAbstractRunner._run
becomes too long. I understand that it will require some duplication, but in theSequentialRunner._run
method, we can add a note explaining why we keep the implementation like that. But adding it toAbstractRunner._run
will overload it even more.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theAbstract
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 readableAbstractRunner._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.