Job Cancelation #998

natibek · 2024-07-23T22:35:43Z

Addressed #822.

…endpoint to gss._SuperstaqClient

…ncel-jobs

richrines1 · 2024-07-24T23:10:56Z

general-superstaq/general_superstaq/superstaq_client.py

+        Args:
+            job_ids: The UUID of the job (returned when the job was created).
+            kwargs:  Extra options needed to fetch jobs.
+                - cq_token: CQ Cloud credentials.


this isn't necessary (we recommend passing credentials when instantiating the client instead of for each call)

Suggested change

- cq_token: CQ Cloud credentials.

richrines1 · 2024-07-25T00:38:18Z

general-superstaq/general_superstaq/superstaq_client.py

+        """Cancel jobs associated with given job ids.
+
+        Args:
+            job_ids: The UUID of the job (returned when the job was created).


nit

Suggested change

job_ids: The UUID of the job (returned when the job was created).

job_ids: The UUIDs of the jobs (returned when the jobs were created).

richrines1 · 2024-07-25T00:38:50Z

general-superstaq/general_superstaq/superstaq_client.py

@@ -192,6 +192,34 @@ def get_job(self, job_id: str) -> dict[str, str]:
        """
        return self.fetch_jobs([job_id])[job_id]

+    def cancel_jobs(
+        self,
+        job_ids: list[str],


Suggested change

job_ids: list[str],

job_ids: Sequence[str],

richrines1 · 2024-07-25T00:40:52Z

qiskit-superstaq/qiskit_superstaq/superstaq_job.py

+                vendor throws error when trying to cancel.
+        """
+        job_ids = self._job_id.split(",")
+        self._backend._provider._client.cancel_jobs(job_ids, kwargs=kwargs)


Suggested change

self._backend._provider._client.cancel_jobs(job_ids, kwargs=kwargs)

self._backend._provider._client.cancel_jobs(job_ids, **kwargs)

richrines1 · 2024-07-25T00:41:19Z

cirq-superstaq/cirq_superstaq/job.py

+            SuperstaqServerException: If unable to get the status of the job from the API.
+        """
+        job_ids = self._job_id.split(",")
+        self._client.cancel_jobs(job_ids, kwargs=kwargs)


Suggested change

self._client.cancel_jobs(job_ids, kwargs=kwargs)

self._client.cancel_jobs(job_ids, **kwargs)

richrines1 · 2024-07-25T00:42:33Z

general-superstaq/general_superstaq/superstaq_client.py

+    def cancel_jobs(
+        self,
+        job_ids: list[str],
+        **kwargs: Any,


does this pass?

Suggested change

**kwargs: Any,

**kwargs: object,

(and ditto anywhere else we're adding Any)

richrines1 · 2024-07-26T22:18:04Z

general-superstaq/general_superstaq/superstaq_client.py

+                response_content = self._handle_response(response)
                if response.ok:
-                    return response
+                    return response_content


thanks for noticing/fixing this! one thought: we probably only want to print warnings in the event of non-retriable status codes. what if we just replaced these lines in _handle_status_codes:

client-superstaq/general-superstaq/general_superstaq/superstaq_client.py

Lines 756 to 759 in 9c99804

try:

json_content = response.json()

except requests.JSONDecodeError:

json_content = None

to

try: json_content = self._handle_response(response) except requests.JSONDecodeError: json_content = None

(and then reverted the change to _handle_response, which shouldn't be necessary anymore)

richrines1 · 2024-07-26T22:23:32Z

general-superstaq/general_superstaq/superstaq_client.py

+                - cq_token: CQ Cloud credentials.
+
+        Returns:
+            A dictionary mapping the job id to the cancellation attempt result or an error message.


might need an update: by the time we get here i think all that's left in the dict is {"succeeded": [<ids>]}

(also might be reasonable to just return the list of ids, instead of the full dictionary)

richrines1 · 2024-07-29T05:05:29Z

general-superstaq/general_superstaq/superstaq_client.py

@@ -797,13 +821,14 @@ def _make_request(self, request: Callable[[], requests.Response]) -> requests.Re
            TimeoutError: If the requests retried for more than `max_retry_seconds`.

        Returns:
-            The `request.Response` from the final successful request call.
+            The 'requests.Response' from the final successful request call.


Suggested change

The 'requests.Response' from the final successful request call.

The `requests.Response` from the final successful request call.

(backticks indicate inline code)

richrines1 · 2024-07-29T05:08:06Z

general-superstaq/general_superstaq/superstaq_client_test.py

+    response1 = mock.MagicMock()
+    response2 = mock.MagicMock()
+    mock_post.side_effect = [response1, response2]
+    response1.ok = False
+    response1.status_code = requests.codes.service_unavailable
+    response2.ok = True


i think this can be streamlined a bit:

Suggested change

response1 = mock.MagicMock()

response2 = mock.MagicMock()

mock_post.side_effect = [response1, response2]

response1.ok = False

response1.status_code = requests.codes.service_unavailable

response2.ok = True

response1 = mock.MagicMock(ok=False, status_code=requests.codes.service_unavailable)

response2 = mock.MagicMock(ok=True)

mock_post.side_effect = [response1, response2]

richrines1 · 2024-07-29T05:15:15Z

cirq-superstaq/cirq_superstaq/job_test.py

+    with mock.patch(
+        "general_superstaq.superstaq_client._SuperstaqClient.cancel_jobs", return_value=None
+    ) as mock_cancel:


does this work?

Suggested change

with mock.patch(

"general_superstaq.superstaq_client._SuperstaqClient.cancel_jobs", return_value=None

) as mock_cancel:

with mock.patch("requests.post", return_value=requests.MagicMock(ok=True)) as mock_post:

The original test passes so the mock seems to be called twice. The suggestion passes too with a minor fix.

great, can we make the same change in qss then?

Can you please explain why the new mock is better? Is the previous one stopping the program calls too early by returning None at _client.cancel_jobs()?

Is the previous one stopping the program calls too early by returning None at _client.cancel_jobs()?

exactly - in general we want to be mocking as little as possible. basically all job.cancel() does is call client.cancel_jobs - if we mock that all we're really testing is the mock itself, not the actual interface between the two methods

in this case the logic is simple enough that it probably doesn't matter. But say we updated _client.cancel_jobs to take a different kind of input, but forgot to update job.cancel() - the previous test wouldn't catch this because we were replacing its call to _client.cancel_jobs() with a mocked function that accepts any input (note in this example the error would hopefully be caught by mypy, but that's not always the case)

Got it. Thank you!

richrines1 · 2024-07-29T16:15:23Z

cirq-superstaq/cirq_superstaq/job.py

@@ -151,6 +151,16 @@ def status(self) -> str:
        self._update_status_queue_info()
        return self._overall_status

+    def cancel(self, **kwargs: Any) -> None:


Suggested change

def cancel(self, **kwargs: Any) -> None:

def cancel(self, **kwargs: object) -> None:

richrines1 · 2024-07-29T16:15:35Z

qiskit-superstaq/qiskit_superstaq/superstaq_job.py

@@ -177,6 +177,16 @@ def _check_if_stopped(self) -> None:
                self._job_id, self._overall_status
            )

+    def cancel(self, **kwargs: Any) -> None:


Suggested change

def cancel(self, **kwargs: Any) -> None:

def cancel(self, **kwargs: object) -> None:

…ncel-jobs

richrines1

lg % a few nits

richrines1 · 2024-07-30T01:10:33Z

cirq-superstaq/cirq_superstaq/job.py

+        """Cancel the current job if it is not in a terminal state.
+
+        Args:
+            kwargs:  Extra options needed to fetch jobs.


Suggested change

kwargs: Extra options needed to fetch jobs.

kwargs: Extra options needed to fetch jobs.

richrines1 · 2024-07-30T01:10:52Z

general-superstaq/general_superstaq/superstaq_client.py

+
+        Args:
+            job_ids: The UUIDs of the jobs (returned when the jobs were created).
+            kwargs:  Extra options needed to fetch jobs.


Suggested change

kwargs: Extra options needed to fetch jobs.

kwargs: Extra options needed to fetch jobs.

richrines1 · 2024-07-30T01:11:32Z

general-superstaq/general_superstaq/superstaq_client.py

+        Raises:
+            SuperstaqServerException: For other API call failures.
+        """
+        json_dict: dict[str, Any] = {


Suggested change

json_dict: dict[str, Any] = {

json_dict: dict[str, list[str]] = {

This breaks a few things. Since job_ids is a Sequence[str] type, we have to cast it to a list. Also, json.dumps returns a string so json_dict["options"] will have type str and not list[str]. I don't think server is designed to handle json_dict["options"] being a list type either. Should I keep it at Any or should I do some type casting to fix these issues?

ah my bad. what about this?

Suggested change

json_dict: dict[str, Any] = {

json_dict: dict[str, str | Sequence[str]] = {

richrines1 · 2024-07-30T01:12:30Z

qiskit-superstaq/qiskit_superstaq/superstaq_job.py

+        """Cancel the current job if it is not in a terminal state.
+
+        Args:
+            kwargs:  Extra options needed to fetch jobs.


Suggested change

kwargs: Extra options needed to fetch jobs.

kwargs: Extra options needed to fetch jobs.

…ncel-jobs

richrines1

lgtm!

Updates docs on some functionality info on features added by #998, #1034, and #1048. Also does a re-run refresh of the notebooks so that the output cells reflect the latest changes (e.g., new targets/backends, #1036, etc). Adds support for Colab and Binder notebook badges as well

natibek added 4 commits July 22, 2024 15:54

Add methods for job cancelation in qss.SuperstaqJob and css.Job, add …

633d76f

…endpoint to gss._SuperstaqClient

tests for coverage

85fc780

Merge branch 'main' of github.com:Infleqtion/client-superstaq into ca…

542cb3b

…ncel-jobs

Handle server response, throw warnings before exceptions

5eb0c4d

natibek requested a review from richrines1 July 26, 2024 21:31

richrines1 reviewed Jul 26, 2024

View reviewed changes

natibek added 2 commits July 27, 2024 05:07

Implement review suggestions

22f8ca4

fix typing

b4bef3a

richrines1 reviewed Jul 29, 2024

View reviewed changes

natibek added 5 commits July 29, 2024 15:58

Review suggestion fixes

df8b4f2

Review suggestion fixes

fad57d2

Merge branch 'main' of github.com:Infleqtion/client-superstaq into ca…

f747363

…ncel-jobs

pylint fix

4165c91

update qss test_cancel

a129e88

richrines1 reviewed Jul 30, 2024

View reviewed changes

natibek added 3 commits July 30, 2024 10:14

Merge branch 'main' of github.com:Infleqtion/client-superstaq into ca…

faede50

…ncel-jobs

fix nits

5c281ff

fix typing for json_dict

ec9d8b0

richrines1 approved these changes Jul 30, 2024

View reviewed changes

bharat-thotakura mentioned this pull request Aug 1, 2024

Version bump to 0.5.22 #1009

Merged

natibek and others added 2 commits August 14, 2024 11:36

Merge branch 'main' into cancel-jobs

93c48a3

fix merge conflict

12bb3a5

natibek merged commit d217a2a into main Aug 15, 2024
19 checks passed

natibek deleted the cancel-jobs branch August 15, 2024 20:54

bharat-thotakura mentioned this pull request Aug 29, 2024

Update access info notebooks with recent updates #1049

Merged

bharat-thotakura mentioned this pull request Sep 9, 2024

Job cancellation not implemented #822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Job Cancelation #998

Job Cancelation #998

natibek commented Jul 23, 2024

richrines1 Jul 24, 2024

richrines1 Jul 25, 2024

richrines1 Jul 25, 2024

richrines1 Jul 25, 2024

richrines1 Jul 25, 2024

richrines1 Jul 25, 2024

natibek Jul 29, 2024

richrines1 Jul 26, 2024

richrines1 Jul 26, 2024

richrines1 Jul 29, 2024

richrines1 Jul 29, 2024

richrines1 Jul 29, 2024

natibek Jul 29, 2024

richrines1 Jul 29, 2024

natibek Jul 29, 2024

richrines1 Jul 29, 2024

natibek Jul 29, 2024

richrines1 Jul 29, 2024

richrines1 Jul 29, 2024

richrines1 left a comment

richrines1 Jul 30, 2024

richrines1 Jul 30, 2024

richrines1 Jul 30, 2024

natibek Jul 30, 2024

richrines1 Jul 30, 2024

richrines1 Jul 30, 2024

richrines1 left a comment

	job_ids: The UUID of the job (returned when the job was created).
	job_ids: The UUIDs of the jobs (returned when the jobs were created).

	self._backend._provider._client.cancel_jobs(job_ids, kwargs=kwargs)
	self._backend._provider._client.cancel_jobs(job_ids, **kwargs)

	self._client.cancel_jobs(job_ids, kwargs=kwargs)
	self._client.cancel_jobs(job_ids, **kwargs)

	try:
	json_content = response.json()
	except requests.JSONDecodeError:
	json_content = None

	The 'requests.Response' from the final successful request call.
	The `requests.Response` from the final successful request call.

	def cancel(self, **kwargs: Any) -> None:
	def cancel(self, **kwargs: object) -> None:

	kwargs: Extra options needed to fetch jobs.
	kwargs: Extra options needed to fetch jobs.

	json_dict: dict[str, Any] = {
	json_dict: dict[str, list[str]] = {

	json_dict: dict[str, Any] = {
	json_dict: dict[str, str \| Sequence[str]] = {

Job Cancelation #998

Job Cancelation #998

Conversation

natibek commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richrines1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richrines1 left a comment

Choose a reason for hiding this comment