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

Using processor: string is incorrect #6427

Open
eliottrosenberg opened this issue Jan 26, 2024 · 5 comments
Open

Using processor: string is incorrect #6427

eliottrosenberg opened this issue Jan 26, 2024 · 5 comments
Assignees
Labels
kind/bug-report Something doesn't seem to work. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on

Comments

@eliottrosenberg
Copy link
Collaborator

Description of the issue

The Using processor: ... string that gets printed when you load a processor is not correct.

See this colab.

Cirq version
1.4.0.dev20240126200039

@senecameeks @wcourtney @NoureldinYosri

@eliottrosenberg eliottrosenberg added the kind/bug-report Something doesn't seem to work. label Jan 26, 2024
@wcourtney
Copy link
Collaborator

wcourtney commented Jan 27, 2024

Thanks for the report! It looks like that message comes from here. I personally don't like this side-effect at all since it's referring to a value that you intentionally ignore in the script. My vote would be to remove it, but I think that it would also go away if the function were used as it appears was intended. e.g., augmenting the colab:

project_id = "i-dont-have-experimental"
processor_id = "SYC02_4B_CANYON"

qcs_objects = get_qcs_objects_for_notebook(project_id, processor_id)

processor = qcs_objects.processor
...

I expect that the incantation provided in the colab comes from the quickstart, so I'd recommend

  1. Update the quickstart with the new form (or some variation) that doesn't result choosing the wrong processor in the utility.
  2. Remove the print statement entirely since there's no reason that the client needs to use the value and they can always print it if they like.

@eliottrosenberg - does that sound alright to you?

@eliottrosenberg
Copy link
Collaborator Author

That sounds good to me! I did not realize that qcs_objects already has device and sampler attributes. That's really convenient but wasn't explained in the quickstart.

@eliottrosenberg
Copy link
Collaborator Author

eliottrosenberg commented Jan 27, 2024

Actually, could we modify get_qcs_objects_for_notebook to take a run_name and device_config_name as optional inputs? That way its sampler attribute could already be the one that the user will use.

@maffoo
Copy link
Contributor

maffoo commented Jan 27, 2024

I've always found the get_qcs_object_for_notebook function to be awkward, starting from the name, and I wonder if we can get away from it with a little bit of refactoring. For example, we already have a get_engine_sampler function, and it would be great if people can just do

sampler = cirq_google.get_engine_sampler(device, run_name, device_config)

The returned sampler has a processor attribute that returns the processor, so that one simple call should be all one needs to get started taking data.

@wcourtney
Copy link
Collaborator

On second look, I think there are two different use cases that we're trying to combine.

  1. The quickstart tutorial is meant to familiarize the user with the resources and how they relate. In this case, we must begin with an engine object to list processors and can follow the resource tree as we introduce concepts. For this, we should probably just use the more straight-forward get_engine, which would also remove the message at the heart of this issue (Using processor: string is incorrect #6427).

  2. After the quickstart, the user is pointed to a template colab "as a base for your own explorations". IMO, is a good place to introduce a simplified setup when the user already knows what they want and just needs a simple configuration. Here, we should discuss the merits of updating get_qcs_objects_for_notebook to accept the run_name and device_config vs using the get_engine_* methods. It looks like this notebook could use a more thorough review and I'd rather track that work in a new issue.

@verult verult added the triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on label Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug-report Something doesn't seem to work. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on
Projects
None yet
Development

No branches or pull requests

5 participants