Skip to content
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

Add error details for failed remote job #370

Merged
merged 12 commits into from
Apr 20, 2020
3 changes: 3 additions & 0 deletions .github/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@

<h3>Improvements</h3>

* Add details to the error message for failed remote jobs.
[(#370)](https://github.com/XanaduAI/strawberryfields/pull/370)

<h3>Bug fixes</h3>

<h3>Contributors</h3>
Expand Down
1 change: 1 addition & 0 deletions strawberryfields/api/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ def get_job(self, job_id: str) -> Job:
id_=response.json()["id"],
status=JobStatus(response.json()["status"]),
connection=self,
meta=response.json()["meta"],
)
raise RequestFailedError(
"Failed to get job: {}".format(self._format_error_message(response))
Expand Down
21 changes: 18 additions & 3 deletions strawberryfields/api/job.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 or {}

self.log = create_logger(__name__)

Expand Down Expand Up @@ -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)
Copy link
Contributor

@antalszava antalszava Apr 20, 2020

Choose a reason for hiding this comment

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

Logically for me, the meta data belongs to the status of the Job. At least this attribute will change the similarly to the change in status and as far as I understand it, is meant to provide more details about the status of the Job.

However, making this change in abstraction might not be so easy (would involve restructuring JobStatus, etc.), so I'm happy as is now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 %s metadata: %s", self.id, job_info.meta)
if self._status == JobStatus.COMPLETED:
self._result = self._connection.get_job_result(self.id)

Expand Down
4 changes: 2 additions & 2 deletions strawberryfields/engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 format should work just fine as well?

)
self.log.error(message)

Expand Down
12 changes: 9 additions & 3 deletions tests/api/test_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,16 +132,19 @@ def test_get_all_jobs_error(self, connection, monkeypatch):

def test_get_job(self, connection, monkeypatch):
"""Tests a successful job request."""
id_, status = "123", JobStatus.COMPLETED
id_, status, meta = "123", JobStatus.COMPLETED, {"abc": "def"}

monkeypatch.setattr(
requests, "get", mock_return(MockResponse(200, {"id": id_, "status": status.value})),
requests,
"get",
mock_return(MockResponse(200, {"id": id_, "status": status.value, "meta": meta})),
)

job = connection.get_job(id_)

assert job.id == id_
assert job.status == status.value
assert job.meta == meta

def test_get_job_error(self, connection, monkeypatch):
"""Tests a failed job request."""
Expand All @@ -155,7 +158,9 @@ def test_get_job_status(self, connection, monkeypatch):
id_, status = "123", JobStatus.COMPLETED

monkeypatch.setattr(
requests, "get", mock_return(MockResponse(200, {"id": id_, "status": status.value})),
requests,
"get",
mock_return(MockResponse(200, {"id": id_, "status": status.value, "meta": {}})),
)

assert connection.get_job_status(id_) == status.value
Expand Down Expand Up @@ -238,6 +243,7 @@ def test_ping_failure(self, connection, monkeypatch):

assert not connection.ping()


class TestConnectionIntegration:
"""Integration tests for using instances of the Connection."""

Expand Down
8 changes: 5 additions & 3 deletions tests/api/test_remote_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -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, meta={"foo": "bar"})


@pytest.fixture
Expand All @@ -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",
Expand Down Expand Up @@ -90,6 +91,7 @@ def test_run_async(self, connection, prog, job_to_complete):
job.refresh()

assert job.status == JobStatus.COMPLETED.value
assert job.meta == {"foo": "bar"}
assert np.array_equal(job.result.samples, np.array([[1, 2], [3, 4]]))

with pytest.raises(
Expand Down