-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Refactor get sampler #7672
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
base: main
Are you sure you want to change the base?
Refactor get sampler #7672
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7672 +/- ##
=======================================
Coverage 99.37% 99.37%
=======================================
Files 1082 1082
Lines 96708 96750 +42
=======================================
+ Hits 96101 96144 +43
+ Misses 607 606 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
run_name: A unique identifier representing an automation run for the | ||
processor. An Automation Run contains a collection of device | ||
configurations for the processor. | ||
device_config_name: An identifier used to select the processor configuration |
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 removes what I would expect to be the most common use case: specify only a device config name, but not the version (run or snapshot).
@eliottrosenberg - do we really want to support a default config name ("config alias") at all? While defaults make sense to get the "latest" version, a default grid seem more risky and I think are worth forcing explicit selection.
return engine_processor.EngineProcessor(self.project_id, processor_id, self.context) | ||
|
||
def get_sampler( | ||
def get_sampler_from_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.
The methods in Engine
are intended to be easy-to-remember convenience helpers and I'm concerned that over-specialization makes these less usable and more painful to maintain. e.g., if we add another feature with 2 ways to construct a sampler and follow this pattern, these 3 functions will turn into 6 new functions with very long names.
The style guide suggests that defaults are useful to avoid "lots of functions for the rare exceptions", and @eliottrosenberg made it sound like these specialized versions are exceptional.
Would it suffice to just check for mutual exclusivity of the arguments and raise an exception if that's violated? Alternatively, if you find it important to bake this check into the interface, you could alternatively do it with the type system, e.g.:
@dataclass
class Snapshot:
id: str
@dataclass
class Run:
id: str
type DeviceVersion = Snapshot | Run
class Engine
...
def get_sampler(
self,
processor_id: str | list[str],
device_config_name: str,
device_version: DeviceVersion | None = None,
max_concurrent_jobs: int = 100,
) -> cirq_google.ProcessorSampler:
...
WDYT?
device_config_name: str = "", | ||
snapshot_id: str = "", | ||
max_concurrent_jobs: int = 100, | ||
def get_sampler_from_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.
Could this be simplified by adding a sampler()
method on ProcessorConfig
? The equivalent of .get_sampler_from_run_name(...)
would then be .get_config_from_run_name(...).sampler()
. As a bonus, this form localizes scope of each resource configuration, e.g. expanded:
sampler = (
engine.get_processor('willow_123')
.get_config_from_run(run_name='current', config_name='some_cool_grid')
.sampler()
)
This or something like it should probably the the canonical form of retrieval because a sampler should always refer to a fully-qualified config even if the user used defaults to construct it.
Raises: | ||
ValueError: If only one of `run_name` and `device_config_name` are specified. | ||
ValueError: If both `run_name` and `snapshot_id` are specified. | ||
def get_sampler( |
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.
Following on from the above comment regarding the requirement of a device config to build a sampler, should we remove get_sampler
from the processor and instead rely on explicitly grabbing the default configuration to generate the sampler?
Fixes bug 435217087. Refactor
get_sampler
and adds the functionsget_sampler_from_run_name
andget_sampler_from_snapshot_id
get_sampler
: This function no longer requires or acceptsrun_name
andsnapshot_id
get_sampler_from_run_name
:get_sampler_from_snapshot_id
: