-
Notifications
You must be signed in to change notification settings - Fork 326
Add execution API to enable custimization of Mars Task Service #2894
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
Conversation
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from .mars import * |
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.
Is this import necessary?
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.
This import is to register and load the mars execution backend.
|
It might be a silly question but I am wondering, since the new execution API only touches the |
The reason why we need a new execution API is that current
We introduce the new execution API is to simplify the execution backend implementation. Mars provides the optimized subtask graph for the execution backend, and the backend only needs to run the graph. More design details are in the #2893 |
Graph construction and execution can be put into task service together, why not just put the execution in task service, and it's still backend replaceable. |
The execution backend is replaceable, it can be put into the task service, but I think moving the execution backend out of the task service is more clear. For example, third party execution backend can be a separate Python package, it only imports the Also, make the execution api out of task service can restrict the developers not mixing the execution logic with the graph construction logic. We can add some tests to forbid the meta, scheduling, cluster, lifecycle imports in the task service. |
|
I stand with @qinxuye as the word |
Yes, these general execution logic will be extracted to the
The design is to make the execution API as the top level Mars API, it is over other Mars APIs. For the third party execution backends, they do not need to know what the service structure of Mars, Mars do not needs to know how the subtask graph is executed. The last graph in the #2893 shows that only the Mars execution backend has code depdendencis with Mars services. |
I don't see any difference for |
|
It's ridiculous just because some backend may not rely on the service, so we remove it from services. but it's still the part of a service. right? I strongly recommend to integrate the execution api into task service, just because it's indeed a part of a service. |
|
The logic of incref/decref is modified in #2900 , please merge the master branch. |
Thanks. I will merge the latest master. |
qinxuye
left a comment
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.
LGTM
chaokunyang
left a comment
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.
LGTM
wjsi
left a comment
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.
LGTM
What do these changes do?
Introduce the Execution API and makes all the Mars execution logic follows this API.
But, this PR still reserves some legacy APIs on Execution API for Mars compatibility. These APIs are not blocking the Execution API implementation, and will be removed in the future.
async def set_subtask_result(self, subtask_result: SubtaskResult)
This API will be removed in the future, it triggers the Mars execution logic currently.
def get_stage_processors(self)
This API will be removed in the future, it is for the following APIs:
Related issue number
#2893