Add Local implementation of abstract engine classes #4645
Conversation
This PR creates partially implemented sub-classes that define in-memory objects for engine functionality unlikely to be implemented differently, such as reservations, labels, and descriptions: AbstractLocalJob, AbstractLocalProgram, AbstractLocalProcessor, AbstractLocalEngine These classes follow the implementation of the production Engine classes but use local memory to provide equivalent functionality for testing, mocking, and local simulator use. This is PR 2 of a three part series.
dstrain115
requested review from
cduck,
vtomole,
wcourtney and
a team
as code owners
There was a problem hiding this comment.
Reviewed AbstractLocal(Engine|Job). Also see my comments from #4644, as many of the parent-class comments apply to the child class.
Will follow up with AbstractLocal(Processor|Program) reviews.
| """Returns the expected the processor should be available, if set.""" | ||
| return self._expected_recovery_time | ||
|
|
||
| def _create_id(self, id_type: str = 'reservation') -> str: |
There was a problem hiding this comment.
This function header looks like it's for multiple ID types, but the body appears specific to reservations. Do we need to differentiate between types, or is the current behavior OK?
There was a problem hiding this comment.
Changed to resource id. I don't think the exact string matters much, but we can always improve on it later, if someone wants behavior related to resource ids.
There was a problem hiding this comment.
It sounds like this will work from your comment, but to confirm: will the following behave properly?
id1 = self._create_id() # id1 = '.../1/reservation'
id2 = self._create_id(id_type='something_else') # id2 = '.../2/something_else'
id3 = self._create_id() # id3 = '.../3/reservation'
i.e. is it OK that the two 'reservation' IDs don't have consecutive numbers?
There was a problem hiding this comment.
Yeah, this should be fine. I think the reality is that the ids are UUIDs, not consecutive integers. We just want them to be unique. I think the one issue is that jobs should really be children of programs, but I am not going to worry about that right now.
cirq-google/cirq_google/engine/abstract_local_processor_test.py
Outdated
Show resolved
Hide resolved
cirq-google/cirq_google/engine/abstract_local_processor_test.py
Outdated
Show resolved
Hide resolved
|
Looks like there are some conflicts in merging part 1 into this. |
Fixed. The merge conflicts were due to removing the coverage:ignore statements, which was intentional, as they are now covered by the tests in this PR. |
MichaelBroughton
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
the
automerge
CirqBot
removed
the
front_of_queue_automerge
|
At least |
|
or JSON serializability |
This PR creates partially implemented sub-classes that define in-memory objects for engine functionality unlikely to be implemented differently, such as reservations, labels, and descriptions: AbstractLocalJob, AbstractLocalProgram, AbstractLocalProcessor, AbstractLocalEngine These classes follow the implementation of the production Engine classes but use local memory to provide equivalent functionality for testing, mocking, and local simulator use. This is PR 2 of a three part series.
This PR creates partially implemented sub-classes that define
in-memory objects for engine functionality unlikely to be implemented
differently, such as reservations, labels, and descriptions:
AbstractLocalJob, AbstractLocalProgram,
AbstractLocalProcessor, AbstractLocalEngine
These classes follow the implementation of the production
Engine classes but use local memory to provide equivalent
functionality for testing, mocking, and local simulator use.
This is PR 2 of a three part series.