Add EngineProcessorTimeSlot to cirq.google.engine #2794
Conversation
dstrain115
commented
Feb 26, 2020
dstrain115
commented
- This class will be a wrapper to hold timeslots for reservations.
- This class will be a wrapper to hold timeslots for reservations.
| from typing import Optional | ||
|
|
||
|
|
||
| class EngineProcessorTimeSlotType(enum.Enum): |
There was a problem hiding this comment.
Although it varies from the doc, we've coalesced around dropping the "Processor" from this type in the rest of the system. WDYT of EngineTimeSlot/EngineTimeSlotType?
| from typing import Optional | ||
|
|
||
|
|
||
| class EngineProcessorTimeSlotType(enum.Enum): |
There was a problem hiding this comment.
Should this enum be in the client library instead? e.g.,
There was a problem hiding this comment.
Moved this enum into client/..../enum.py
| slot_type: EngineProcessorTimeSlotType = _DEFAULT_SLOT_TYPE | ||
| project_id: Optional[int] = None | ||
| maintenance_title: Optional[str] = None | ||
| maintenance_description: Optional[str] = None |
There was a problem hiding this comment.
The refreshed interface now consistently exposes only primary keys as fields and provides accessors that lazily load the underlying object. Consider keeping that pattern for consistency?
e.g.
https://github.com/quantumlib/Cirq/blob/master/cirq/google/engine/engine_program.py
https://github.com/quantumlib/Cirq/blob/master/cirq/google/engine/engine_job.py
There was a problem hiding this comment.
Used frozen=True instead of adding accessors to have a short, readable class.
| @@ -94,6 +94,23 @@ class Health(enum.IntEnum): | |||
| DOWN = 2 | |||
| UNAVAILABLE = 3 | |||
|
|
|||
| class TimeSlotType(enum.IntEnum): | |||
There was a problem hiding this comment.
In the API, the TimeSlotType nested inside of QuantumTimeSlot. Does this need to match to work appropriately with the gRPC client, or is the plan to translate these? I think that this code was originally generated from the API protos, so I'm guessing that they should be in sync.
| start_seconds: int | ||
| end_seconds: int | ||
| slot_type: enums.QuantumProcessor.TimeSlotType = _DEFAULT_TYPE | ||
| project_id: Optional[int] = None |
There was a problem hiding this comment.
project_id should be a string..
| class EngineTimeSlot: | ||
| """A python wrapping of a Quantum Engine timeslot. | ||
| """ | ||
| start_seconds: int |
There was a problem hiding this comment.
WDYT of datetime for start and end times? Is there a reason to prefer seconds?
There was a problem hiding this comment.
Well, this would require some additional conventions. The proto defines this as a Timestamp which is seconds since epoch:
https://cs.corp.google.com/piper///depot/google3/google/protobuf/timestamp.proto?l=125
If we define this in datetime, we have to figure out how to translate this to datetime with timezone information and watch out that we don't screw up when serializing / deserializing. (For instance, if one researcher serializes to JSON and another in the project but in a different time zone gets the timeslot and uses it.)
It's totally doable, but it adds some complexity here. Are you sure we need this information?
There was a problem hiding this comment.
We don't need it, but I'm concerned about the usability if this is the interface for viewing a processor's schedule. Time slots are read-only, and IIUC datetime.fromtimestamp() will convert to local time, so it sounds like we're just pushing that onto the users. Not a big deal yet though and I can wait to see where this goes.
There was a problem hiding this comment.
Ok, changed to datetime. I figure its easier to change now than later, and we can probably create a from_proto() function once the protos are in cirq.
