Update RFC-0002 to remove unused fields for implementation#8
Update RFC-0002 to remove unused fields for implementation#8mprahl merged 1 commit intomlflow:mainfrom
Conversation
|
@B-Step62 @TomeHirata could you please take a look? I started on the database schema and proto changes implementation and saw some unnecessary fields. |
| status: JobStatus # executors return only terminal statuses from wait_for_job() | ||
| result: str | None = None | ||
| error_message: str | None = None | ||
| result: str | None = None # canonical terminal payload for both success and failure/timeout outcomes |
There was a problem hiding this comment.
This is a json string, correct? What was the reason to make it str instead of dict[str, Any] in the dataclass?
There was a problem hiding this comment.
The current dataclass has it as a string in mlflow/server/jobs/utils.py:
@dataclass
class JobResult:
succeeded: bool
result: str | None = None # serialized JSON string
is_transient_error: bool | None = None
error: str | None = None@TomeHirata I can change it but existing code expects a string.
| job_id: str, | ||
| status: JobStatus, | ||
| result: str | None = None, | ||
| error_message: str | None = None, |
There was a problem hiding this comment.
Why do we want to remove error_message from this API? We still have error message in JobResult
There was a problem hiding this comment.
I was initially leaning toward removing error_message here because the current database model does not have a separate error_message column and instead uses result for both success and failure payloads. After thinking it through more, I think it still makes sense to keep error_message on the API surface, since it represents a different semantic field even if storage normalizes both cases into the same persisted column.
There was a problem hiding this comment.
Yeah I think it makes more sense to separate result from error message. Result should be ideally an outcome of the successful jobs.
mprahl
force-pushed
the
update-exec-plugin
branch
from
323355a to
a25ad1e
Compare
|
@TomeHirata updated! |
error_message does not need to be a new database schema column. Reuse the existing result field as the current implementation does. Additionally, the holder_id in the scheduler lease will not be used. Signed-off-by: mprahl <mprahl@users.noreply.github.com>
mprahl
force-pushed
the
update-exec-plugin
branch
2 times, most recently
from
7575adf to
aa34822
Compare
2 participants
error_message does not need to be a new database schema column. Reuse
the existing result field as the current implementation does.
Additionally, the holder_id in the scheduler lease will not be used.