-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow external users to use snapshot_id #6739
Allow external users to use snapshot_id #6739
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6739 +/- ##
=======================================
Coverage 97.83% 97.83%
=======================================
Files 1077 1077
Lines 92672 92742 +70
=======================================
+ Hits 90669 90738 +69
- Misses 2003 2004 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM. However, I don't think I'm at the point yet in my understanding of Cirq and the internal systems that I'm qualified to review this, so I think this needs someone else's approval.
@@ -600,7 +650,7 @@ def test_create_job_with_run_name_and_device_config_name( | |||
priority=10, | |||
processor_selector=quantum.SchedulingConfig.ProcessorSelector( | |||
processor='projects/proj/processors/processor0', | |||
device_config_selector=quantum.DeviceConfigSelector(), | |||
device_config_selector=quantum.DeviceConfigSelector(run_name=""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change needed? I wouldn't expect this PR's change to affect the existing tests of run_name
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a side-effect of moving run_name
into a oneof
"Oneof Impact: When you assign an empty string to a string field within a oneof, protocol buffers interpret this as explicitly setting that field to a value (even if it's an empty value). This triggers the oneof to recognize that field as the active one."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
snapshot_id: A unique identifier for an immutable snapshot reference. | ||
A snapshot contains a collection of device configurations for the | ||
processor. Both `snapshot_id` and `run_name` should not be set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this above the device_config_name
. This should be next to the run name
. Also, I feel it is good to put "Both snapshot_id
and run_name
should not be set." in run_name docstring as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % a nit.
A follow up to PR, CL
Allows external users to set a snapshot_id