Initialize processor sampler with the default device config key #6521
Conversation
This reverts commit bcd3533.
senecameeks
requested review from
wcourtney,
vtomole,
cduck,
verult and
a team
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6521 +/- ##
==========================================
- Coverage 97.78% 97.77% -0.01%
==========================================
Files 1105 1105
Lines 95112 95154 +42
==========================================
+ Hits 93003 93041 +38
- Misses 2109 2113 +4 ☔ View full report in Codecov by Sentry. |
senecameeks
changed the title
|
My local env was missing some dependencies which made testing locally difficult. I was able to install the write dependencies buried in |
| super().__init__() | ||
| self._processor = quantum.QuantumProcessor() | ||
|
|
||
| async def get_processor_async( |
There was a problem hiding this comment.
This doesn't do anything async and doesn't seem to be called by the system under test. Is it needed? It seems like getProcessor() could just return the value instead.
There was a problem hiding this comment.
The fake needed to match the method signature of its parent class, otherwise errors arose or it would default to the super class. My initial attempt was to do what you described but that caused errors. Regardless, this chunk was removed in the update in favor of a mock client. :)
|
|
||
|
|
||
| def test_get_sampler_loads_processor_with_default_device_configuration() -> None: | ||
| client = FakeEngineClient() |
There was a problem hiding this comment.
This looks almost the same as a mock, which would skip creating the FakeEngineClient class; wdyt?
client = mock.Mock(engine_client.EngineClient)
client.get_processor.return_value = quantum.QuantumProcessor(
...
)| """ | ||
| processor = self._inner_processor() | ||
| if processor is not None: |
There was a problem hiding this comment.
self._inner_processor() should never return None according to the type hint.
There was a problem hiding this comment.
Good point, although there are null checks on _inner_processor() in other parts of the code :)
I added a bit of logic that initializes the sampler with the default values if get_sampler is improperly parameterized with one of either run_name or config_alias, and thus would raise a ValueError.
Instead, we correct the device config key for them and print out that the default value are being used. @wcourtney , WDYT? Right now the device spec being returned is always the Processor's default, but this will be useful in the future as the device spec reflects the device_config_key specified.
| # to the Processor's default values. | ||
| if not run_name or not device_config_name: | ||
| logging.info( | ||
| "Cannot specify only one of `run_name` and `device_config_name`. " |
There was a problem hiding this comment.
If a user tried to specify a config then they probably didn't intend to use the default. If we automatically fix it for them, then they may not notice and wind up running the experiment with a different config than they expected. They should acknowledge the issue by fixing the incorrect config, so we should probably still raise an error.
There was a problem hiding this comment.
I considered this, but thought logging that we're switching to the default values would be sufficient since there's precedent for switching to a simulator if the user is unable to connect to the quantum_engine. Example.
But, I think this is a good concern so I opted to only initialize with the default value if run_name and config_alias are not specified.
| @@ -14,13 +14,14 @@ | |||
|
|
|||
| import datetime | |||
|
|
|||
| import logging | |||
There was a problem hiding this comment.
Do you know if there's a policy on logging in Cirq? I only see it used for tests in Google-managed code (cirq-core and cirq-google). I do see some examples of print statements coupled with a verbosity flag, e.g.:
…tumlib#6521) * add default device config key to grid device metadata * Revert "add default device config key to grid device metadata" This reverts commit bcd3533. * initialize sampler with default device config key and add tests * add properties to sampler * dedupe DeviceConfigK, onewas renamed to DeviceConfigSelector * spelling * coverage * address comments * add fake engine context and client for testing * lint * use new FakeEngineContext * typecheck * lint , local env not catching CI errors * signature of fakes match real impls * add logic to initialize default values instead of throwing value error * lint * rm logging and prefer to throw value error
DeviceConfigKeyproto wrapper class.run_nameandconfig_aliasfrom the innerQuantumProcessor.ProcessorSelectorrenamed its field todevice_config_selector, this PR fixes the client code so it aligns with the QuantumEngine representation of a processor selector. This also de-dupes the classDeviceConfigKey.