Move core/local/ExecutionScheduler to execution/Scheduler
#2812
Conversation
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
Codecov Report
@@ Coverage Diff @@
## master #2812 +/- ##
==========================================
- Coverage 76.88% 76.54% -0.35%
==========================================
Files 218 217 -1
Lines 16650 16664 +14
==========================================
- Hits 12802 12755 -47
- Misses 3041 3092 +51
- Partials 807 817 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
na--
force-pushed
the
move-things-to-execution
branch
from
ae83e9c
to
ef18b38
Compare
olegbespalov
previously approved these changes
Dec 14, 2022
oleiade
previously approved these changes
Dec 14, 2022
There was a problem hiding this comment.
Very good work 👍🏻 I'm glad to see the move to a dedicated execution package happen 🎉 🦖 I've left a suggestion to maybe find a more suiting name for the Scheduler as we're at it, but I've really no strong opinion on this, and you should feel free to walk past it 🙇🏻
na--
force-pushed
the
refactor-run-status
branch
from
62eb4f9
to
b456ed7
Compare
na--
force-pushed
the
move-things-to-execution
branch
from
8e8f1aa
to
947ecb1
Compare
na--
force-pushed
the
refactor-run-status
branch
2 times, most recently
from
8c360a1
to
4077da3
Compare
na--
force-pushed
the
move-things-to-execution
branch
from
947ecb1
to
3b44fa2
Compare
na--
force-pushed
the
refactor-run-status
branch
from
4077da3
to
bcceeb3
Compare
na--
force-pushed
the
move-things-to-execution
branch
from
3b44fa2
to
e5b8879
Compare
na--
force-pushed
the
refactor-run-status
branch
from
be3aa5f
to
2955dcc
Compare
na--
force-pushed
the
move-things-to-execution
branch
from
e5b8879
to
ef09f1b
Compare
We also remove the lib.ExecutionScheduler interface because it only ever had a single implementation and it's unlikely we'll ever need more than that. Distributed execution will be implemented another way and we should not mock the execution scheduler, we should mock the parts it moves (e.g. the Runner and Executors, if needs be).
na--
force-pushed
the
move-things-to-execution
branch
from
ef09f1b
to
a42bdc7
Compare
oleiade
previously approved these changes
Jan 24, 2023
The expanded capabilities will be necessary for changes in upcoming PRs
na--
force-pushed
the
move-things-to-execution
branch
from
a42bdc7
to
9f050f7
Compare
codebien
approved these changes
Jan 24, 2023
oleiade
approved these changes
Jan 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
...and also move and expand the special context wrapper that
test.abort()uses to interrupt the test and store the error internally. Its improved capabilities will be required for #1889This is built on top of #2810 and is also the first step towards addressing #1302. There is still a long way to go before we can move all of the execution-related things out of
lib/, mostly because it will be quite tricky to both have a nice module structure and avoid import loops 😞 Even now, parts ofexecution/(esp. the tests) still importlib/executor/, whilelib/executorimports the parts oflib/we want to move toexecution/... 😭 I worked around the issue this time, though that might not always work as well as it did this time.In any case, I just made this change to get the process of cleaning up
lib/started. Between this PR and the next one to address #1889, it's much easier to remove thecore/package than it is to cleanly split apartlib/. Now, we have at least a place for these things, even if we can't move everything quite yet.