-
Notifications
You must be signed in to change notification settings - Fork 216
Add error details for failed remote job #370
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
Changes from 8 commits
71e9d2b
e1ec6b0
8a87a98
c83bb86
535d2ed
5339d06
71bee78
076d7b7
bb926f6
f33a17c
899229e
4c81ff2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,13 +73,15 @@ class Job: | |
| status (strawberryfields.api.JobStatus): the job status | ||
| connection (strawberryfields.api.Connection): the connection over which the | ||
| job is managed | ||
| meta (dict[str, str]): metadata related to job execution | ||
| """ | ||
|
|
||
| def __init__(self, id_: str, status: JobStatus, connection): | ||
| def __init__(self, id_: str, status: JobStatus, connection, meta: dict = None): | ||
| self._id = id_ | ||
| self._status = status | ||
| self._connection = connection | ||
| self._result = None | ||
| self._meta = meta if meta is not None else {} | ||
|
|
||
| self.log = create_logger(__name__) | ||
|
|
||
|
|
@@ -119,14 +121,27 @@ def result(self) -> Result: | |
| ) | ||
| return self._result | ||
|
|
||
| @property | ||
| def meta(self) -> dict: | ||
| """Returns a dictionary of metadata on job execution, such as error | ||
| details. | ||
|
|
||
| Returns: | ||
| dict[str, str] | ||
| """ | ||
| return self._meta | ||
|
|
||
| def refresh(self): | ||
| """Refreshes the status of an open or queued job, | ||
| """Refreshes the status and metadata of an open or queued job, | ||
| along with the job result if the job is newly completed. | ||
| """ | ||
| if self._status.is_final: | ||
| self.log.warning("A %s job cannot be refreshed", self._status.value) | ||
| return | ||
| self._status = JobStatus(self._connection.get_job_status(self.id)) | ||
| job_info = self._connection.get_job(self.id) | ||
| self._status = JobStatus(job_info.status) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logically for me, the However, making this change in abstraction might not be so easy (would involve restructuring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree with the general idea, and also that it would be a relatively big change, so it seems best to tackle that in the future 🙂 |
||
| self._meta = job_info.meta | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be a good idea to log the retrieved metadata at the DEBUG level here? Combined with #360, this will allow retrieved job metadata to appear within the users logs (if they decide to configure it). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, this has been added! Let me know if you think the message formatting needs to be changed. |
||
| self.log.debug("Job {} metadata: {}".format(self.id, job_info.meta)) | ||
pauljxtan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if self._status == JobStatus.COMPLETED: | ||
| self._result = self._connection.get_job_result(self.id) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -547,8 +547,8 @@ def run(self, program: Program, *, compile_options=None, **kwargs) -> Optional[R | |
|
|
||
| if job.status == "failed": | ||
| message = ( | ||
| "The remote job %s failed due to an internal " | ||
| "server error. Please try again." % job.id | ||
| "The remote job {} failed due to an internal " | ||
| "server error. Please try again. {}".format(job.id, job.meta) | ||
|
Comment on lines
+550
to
+551
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These were previously made so such that they can be used for logging, but here the message is already pre-defined, so I guess |
||
| ) | ||
| self.log.error(message) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,16 +35,17 @@ class MockServer: | |
| def __init__(self): | ||
| self.request_count = 0 | ||
|
|
||
| def get_job_status(self, _id): | ||
| def get_job(self, _id): | ||
| """Returns a 'queued' job status until the number of requests exceeds a defined | ||
| threshold, beyond which a 'complete' job status is returned. | ||
| """ | ||
| self.request_count += 1 | ||
| return ( | ||
| status = ( | ||
| JobStatus.COMPLETED | ||
| if self.request_count >= self.REQUESTS_BEFORE_COMPLETED | ||
| else JobStatus.QUEUED | ||
| ) | ||
| return Job(id_="123", status=status, connection=None) | ||
|
||
|
|
||
|
|
||
| @pytest.fixture | ||
|
|
@@ -56,7 +57,7 @@ def job_to_complete(connection, monkeypatch): | |
| mock_return(Job(id_="123", status=JobStatus.OPEN, connection=connection)), | ||
| ) | ||
| server = MockServer() | ||
| monkeypatch.setattr(Connection, "get_job_status", server.get_job_status) | ||
| monkeypatch.setattr(Connection, "get_job", server.get_job) | ||
| monkeypatch.setattr( | ||
| Connection, | ||
| "get_job_result", | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.