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

get_qcs_objects_for_notebook needs cleanup and bugfixes #4587

Open
tanujkhattar opened this issue Oct 19, 2021 · 11 comments
Open

get_qcs_objects_for_notebook needs cleanup and bugfixes #4587

tanujkhattar opened this issue Oct 19, 2021 · 11 comments
Assignees
Labels
area/notebook-testing good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. kind/bug-report Something doesn't seem to work. kind/docs Documentation related problems, ideas, requests kind/health For CI/testing/release process/refactoring/technical debt items priority/p2 Next release should contain it triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add

Comments

@tanujkhattar
Copy link
Collaborator

Description of the issue
get_qcs_objects_for_notebook was added in #4286 and is super useful to abstract out the authentication logic for accessing QCS. However, I think there are a few rough edges -- from a user perspective, I haven't dug into the technical details yet.

  1. In the getting started with QCS tutorial, the object is used for authentication and verification that the authentication went through (device_sampler.is_signed_in) but after that, there's no usage of the device_sampler object. This doesn't look like the intended usage of an object specifically has a device and sampler objects ?
  2. It looks like sqrt_iswap gateset string is hardcoded deep inside the get_qcs_objects_for_notebook method. This also looks like a bug and something that shouldn't be intended?
    sampler = get_engine_sampler(processor_id, gate_set_name="sqrt_iswap")

Cirq version
0.13.0

@tanujkhattar tanujkhattar added kind/health For CI/testing/release process/refactoring/technical debt items kind/docs Documentation related problems, ideas, requests labels Oct 19, 2021
@viathor viathor added the triage/discuss Needs decision / discussion, bring these up during Cirq Cynque label Oct 20, 2021
@tanujkhattar tanujkhattar changed the title get_qcs_objects_for_notebook needs cleanup get_qcs_objects_for_notebook needs cleanup and bugfixes Oct 21, 2021
@tanujkhattar tanujkhattar added the kind/bug-report Something doesn't seem to work. label Oct 21, 2021
@tanujkhattar
Copy link
Collaborator Author

The call to get_engine_device(processor_id) in get_qcs_objects_for_notebook is wrong as it doesn't pass any gateset info / project_id parameters and hence returns an empty device which cannot be used for validation.

@dstrain115 dstrain115 added priority/p2 Next release should contain it triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Oct 27, 2021
@mpharrigan
Copy link
Collaborator

(1) seems fine

(2) -- I think the goal is to remove gateset as a required arg for get_engine_sampler. it's weird to have to pick your target gateset instead of getting a device and it telling you its supported gateset(s)

(3) this seems like a bug in get_engine_device. we shouldn't be returning empty devices

@dstrain115
Copy link
Collaborator

Did we fix this already in another issue? If not, this seems like some doc cleanup that should likely be addressed if possible before 1.0. Tentatively assigning before 1.0 to this issue.

@maffoo
Copy link
Contributor

maffoo commented Apr 6, 2022

This is partly fixed by #5045, which cleans up the logic including removing hard-coded sqrt_iswap. We could potentially refactor things a bit to address the fact that the start.ipynb notebook doesn't actually use the sampler or device returned by the helper function, but rather just checks that we are authenticated and then connects to quantum engine later by calling cirq_google.get_engine(). So we could perhaps split out the authentication stuff into a separate helper function to be called in situations like this.

@maffoo maffoo self-assigned this Apr 6, 2022
@maffoo
Copy link
Contributor

maffoo commented Apr 12, 2022

I think we can address some of the shortcomings of get_qcs_objects_for_notebook in #5240. @verult, should I assign this issue to you?

@verult verult assigned verult and unassigned maffoo May 2, 2022
@verult
Copy link
Collaborator

verult commented Jun 14, 2022

Unassigning myself for now in case anyone can get to this before I can! Otherwise happy to take on this when I get some spare cycles.

@verult verult removed their assignment Jun 14, 2022
@mpharrigan
Copy link
Collaborator

@verult what's the status of #5240?

@verult
Copy link
Collaborator

verult commented Jun 14, 2022

#5240 should ideally be replaced by calling a refactored version of get_qcs_objects_for_notebook which would expose

  • the project_id used for signing in and
  • the processor_id

in addition to (or maybe in place of) existing return values.

@maffoo maffoo self-assigned this Jun 22, 2022
@dstrain115 dstrain115 self-assigned this Jul 14, 2022
@dstrain115
Copy link
Collaborator

I'm taking this on in #5759

@dstrain115
Copy link
Collaborator

Processor_id and project_id are now returned in #5759 and the function is cleaned up.

I have a follow up in #5766 that attempts authentication only if virtual=False and tests this out in cirq_google notebooks to enable better testing and will close this after that PR is in.

@dstrain115
Copy link
Collaborator

@wcourtney @verult I think we can close this now. Do you agree?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/notebook-testing good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. kind/bug-report Something doesn't seem to work. kind/docs Documentation related problems, ideas, requests kind/health For CI/testing/release process/refactoring/technical debt items priority/p2 Next release should contain it triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add
Projects
None yet
Development

No branches or pull requests

8 participants