-
Notifications
You must be signed in to change notification settings - Fork 31
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
Translate all job statuses to Qiskit terminology in client #1062
Conversation
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.
Also, when changing records in existing db you need to take care of entries in DB that are already there and convert them too.
Other hotfix option to close this issue would be just to do mapping in client, so you will not need to change anything on a backend and just show in client mapped statuses. Something like
def gateway_to_runtime_statuses(gateway_status):
mapping = {
"SUCCEEDED": "DONE"
}
return mapping.get(gateway_status, f"APIStatus[{gateway_status}]")
Basically what migrations are doing:
|
@@ -127,7 +127,7 @@ def __init__(self, client: JobSubmissionClient): | |||
self._job_client = client | |||
|
|||
def status(self, job_id: str): | |||
return self._job_client.get_job_status(job_id) | |||
return self._job_client.get_job_status(job_id).value | |||
|
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.
Since status
is an unimplemented BaseJobClient
method, I think both the ray and gateway job clients should return a string indicating the status. I updated this in this PR because it affects how I do the status translation in the user-facing job client
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.
so, you basically do not want to return status class? and return status string?
I have no objections, just to confirm :)
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 figured since the gateway job client returns a string, we may as well return a string from the ray job client as well, since they both derive from the same base class. Not a big deal, but I thought it was a clean simplification.
@@ -473,7 +473,7 @@ def __init__( | |||
|
|||
def status(self): | |||
"""Returns status of the job.""" | |||
return self._job_client.status(self.job_id) | |||
return _map_status_to_serverless(self._job_client.status(self.job_id)) |
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.
We will do the translation from any supported job client status to Qiskit terminology here in the user-facing Job
client. This is the only place we'll do the translation. Everything underneath this will be Ray terminology.
@@ -496,7 +496,7 @@ def result(self, wait=True, cadence=5, verbose=False): | |||
if wait: | |||
if verbose: | |||
logging.info("Waiting for job result.") | |||
while not self._in_terminal_state(): | |||
while not self.in_terminal_state(): |
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.
We don't have a JobStatus
object like ray that we can query like Ray's JobStatus.is_terminal
, so we will make this Job.in_terminal_state
method public, so clients have a way to query whether the job has finished.
a823322
to
aa0c2b4
Compare
@IceKhan13 , I've fixed this up as you suggested. I don't currently have an idea about this strange sphinx error, but Im looking into it. Doesn't seem to be related to the work in this PR, so it's a little confusing |
Hmm, I fixed it locally by removing |
940ebf4
to
4f3520e
Compare
try: | ||
return status_map[status] | ||
except KeyError as exc: | ||
raise KeyError(f"Cannot interpret unknown job status: {status}") from exc |
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.
Maybe we shouldn't error, maybe we should just print the status like you suggested? Not really sure
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 think printing is more user friendly :)
merge main and you will be good :) |
PR looks good! simple and clean like all great things 😃 |
Any particular reason you included the |
c1dbf7a
to
847c314
Compare
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
…iskit#1062)" This reverts commit 7a62247.
Fixes #1039
Summary
In this PR, we translate job statuses from all job clients to Qiskit terminology. Rather than changing the DB structure (currently uses ray status names), we will just do a translation in the serverless client when the user requests the status.
Serverless --> Qiskit nomenclature changes
Pending --> Initializing
Stopped --> Canceled
Succeeded --> Done
Failed --> Error