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

Retrieve old jobs #1099

Merged
merged 11 commits into from
Nov 13, 2024
Merged

Retrieve old jobs #1099

merged 11 commits into from
Nov 13, 2024

Conversation

natibek
Copy link
Contributor

@natibek natibek commented Oct 29, 2024

Addressing #727. For qiskit_superstaq, this enables users to retrieve a job without specifying a backend using the qss.SuperstaqProvider object.

@natibek natibek requested review from richrines1 and vtomole October 29, 2024 22:53
Copy link
Member

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

Did you get rid of the other ways to retrieve old jobs in this PR?

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Raises:
~gss.SuperstaqServerException: If there was an error accessing the API.
"""
job = self._client.fetch_jobs([job_id])
Copy link
Contributor

Choose a reason for hiding this comment

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

this should also be able to multi-circuit jobs (via a comma-separated job_id), e.g.

Suggested change
job = self._client.fetch_jobs([job_id])
jobs = self._client.fetch_jobs(job_id.split(","))

(and with the logic below updated accordingly)

in this case we'll also probably want to check that all the retrieved jobs have the same target, and if not throw an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We opted to fetch one job to match the implementation in cirq_superstaq. Should that also be able to retrieve multi-circuit jobs?

Copy link
Contributor

@richrines1 richrines1 Nov 5, 2024

Choose a reason for hiding this comment

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

css.Service.get_job() accepts comma-separated job ids - e.g. this works for me:

service = css.Service()
job = service.create_job(
    [
        cirq.Circuit(cirq.measure(cirq.q(0))),
        cirq.Circuit(cirq.X(cirq.q(0)), cirq.measure(cirq.q(0)))
    ],
    target="ss_unconstrained_simulator",
    method="dry-run",
)
print(job.job_id())  # will have a comma, e.g. "bc58eb54-5787-4328-bd45-a53f0cd609c,66f63a82-822d-43f0-8c1d-dce1923ec67"

new_job = service.get_job(job.job_id())
print(new_job.counts(0))  # {"0": 1000}
print(new_job.counts(1))  # {"1": 1000}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad! I looked at the docstring for get_job() and it seemed like it only took a single job id.

  def get_job(self, job_id: str) -> css.job.Job:
        """Gets a job that has been created on the Superstaq API.

        Args:
            job_id: The UUID of the job. Jobs are assigned these numbers by the server during the
            creation of the job.

        Returns:
            A `css.Job` which can be queried for status or results.

        Raises:
            ~gss.SuperstaqServerException: If there was an error accessing the API.
        """
        return css.job.Job(client=self._client, job_id=job_id)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah that's confusing lol. i think the idea was that a job id with commas is still just a single "job id", but for a multi-circuit job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that makes sense.

@richrines1
Copy link
Contributor

Did you get rid of the other ways to retrieve old jobs in this PR?

are we sure we want to drop backend.retrieve_jobs? this is technically a breaking change (though not sure how much use it gets so maybe doesn't matter)

@natibek
Copy link
Contributor Author

natibek commented Nov 6, 2024

Did you get rid of the other ways to retrieve old jobs in this PR?

are we sure we want to drop backend.retrieve_jobs? this is technically a breaking change (though not sure how much use it gets so maybe doesn't matter)

I can try looking for instances of its use. The only one I found through test failures was in a notebook. @vtomole what do you think?

@vtomole
Copy link
Member

vtomole commented Nov 7, 2024

From dev meeting: Add deprecation warning for usage and remove later.

@@ -164,6 +164,30 @@ def backends(
superstaq_backends.append(self.get_backend(backend.target))
return superstaq_backends

def retrieve_job(self, job_id: str) -> qss.SuperstaqJob:
Copy link
Member

Choose a reason for hiding this comment

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

Change to get_job for cirq-superstaq parity

Raises:
~gss.SuperstaqServerException: If there was an error accessing the API.
"""
warnings.warn(
Copy link
Member

Choose a reason for hiding this comment

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

Use Cirq's deprecation infrastructure. Here's an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it worth having our own deprecation warning decorator in gss?

Copy link
Member

Choose a reason for hiding this comment

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

It is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What version should we say is the deadline for deprecation?

Copy link
Member

Choose a reason for hiding this comment

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

Let's do 0.5.40. I made it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double checking. This is the warning body we will have

f'{qualname} was used but is deprecated.\n' f'It will be removed in cirq {deadline}.\n' f'{fix}\n'.
Given that the message will state that the function will be deprecated for a certain cirq version, should we use it for qss?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. What we have now is fine then.

Copy link
Member

@vtomole vtomole left a comment

Choose a reason for hiding this comment

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

LGTM

@natibek natibek merged commit cbb1552 into main Nov 13, 2024
21 checks passed
@natibek natibek deleted the retrieve-old-jobs branch November 13, 2024 21:14
@bharat-thotakura bharat-thotakura linked an issue Nov 14, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need a backend to retrieve an old job
3 participants