Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

[Rest API] Add task state; Add job's retry details; Refine job config #2306

Merged
merged 10 commits into from
Mar 29, 2019

Conversation

sunqinzheng
Copy link
Contributor

@sunqinzheng sunqinzheng commented Mar 11, 2019

  • add task state field
  • add job's retry details field
  • refine job config api
    • use request's Accept header to determine whether json or yaml will be sent.
    • BREAK CHANGE: fix the bug that old job config api may return an escaped json string instead of a json object

@sunqinzheng sunqinzheng requested a review from fanyangCS March 20, 2019 10:18
@sunqinzheng sunqinzheng requested a review from yqwang-ms March 20, 2019 10:29
@sunqinzheng sunqinzheng marked this pull request as ready for review March 20, 2019 10:29
@coveralls
Copy link

coveralls commented Mar 20, 2019

Coverage Status

Coverage decreased (-0.03%) to 52.607% when pulling dc5e23b on qinsu/rest into 6e0a52a on master.

retries: retriedCount,
// sum of retries
retries: retries,
retryDetails: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better name to be retryBreakdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "details" is better...

docs/rest-server/API.md Outdated Show resolved Hide resolved
docs/rest-server/API.md Outdated Show resolved Hide resolved
docs/rest-server/API.md Outdated Show resolved Hide resolved
docs/rest-server/API.md Outdated Show resolved Hide resolved
// Job failed due to user or unknown error
user: userRetries,
// Job failed due to system/plaform error
system: systemRetries,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"platform" is better than "system" due to:

  1. More accurate, user job may also be a "system"
  2. Our PAI is "platform" for AI, not "system" for AI
  3. There are PaaS, but no "system" as a Service

so can you replace all "system" words with "platform" in this PR?

Copy link
Contributor Author

@sunqinzheng sunqinzheng Mar 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find that "system" is used in rest api document (https://github.com/Microsoft/pai/blob/master/docs/rest-server/API.md#post-token) multiple times. Shall we replace them as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems there is no need to distingush with user in that doc, so no ambiguity in the doc (the "system" in the doc implies PAI).
So, it is better to replace "in the system" to "in PAI", but it is not necessary, it depends on you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Member

@yqwang-ms yqwang-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you replace all "system" words with "platform" in this PR?

@sunqinzheng
Copy link
Contributor Author

#2382

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants