[cirqflow] Provide concrete ExecutableSpec #4559
Conversation
- writing serialization tests - could be usefull
mpharrigan
requested review from
cduck,
vtomole,
wcourtney and
a team
as code owners
| @@ -197,7 +232,7 @@ def __str__(self) -> str: | |||
| if len(self.executables) > 2: | |||
| exe_str += ', ...' | |||
|
|
|||
| return f'QuantumExecutable(executables=[{exe_str}])' | |||
There was a problem hiding this comment.
this was an unrelated bug I found while updating the tests
|
Lint failure is in an unrelated file |
|
@MichaelBroughton ptal |
| with pytest.raises(TypeError, match='unhashable.*'): | ||
| hash(KeyValueExecutableSpec(executable_family='', key_value_pairs=[('name', 'test')])) |
There was a problem hiding this comment.
Nit: Why have this check ?
There was a problem hiding this comment.
I want all these dataclasses to be immutable as possible. We use hashability as a proxy. If you pass a list of key value pairs instead of a tuple of key value pairs, it should raise an exception.
There was a problem hiding this comment.
hashing a list will always raise an exception. To me this feels like we're breaking the API requirements and then testing for the failures that emerge because of that, which doesn't seem like a great practice, but there's only one such instance so I'm easy either way.
There was a problem hiding this comment.
we do that all the time to hit our coverage requirements
There was a problem hiding this comment.
It's also not "hashing a list". It's hashing a class whose constructor has been provided a list. If we had a check like this for ParamResolver we wouldn't have #4327 :)
|
@MichaelBroughton ; pushed fixes. PTAL if you want. Otherwise I'll merge this afternoon |
mpharrigan
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
`ExecutableSpec` is a newly-introduced abstract class. The design was each application would subclass it for application-dependent data attributes. 1. Writing all the serialization tests is a nightmare without a full-fledged lives-in-Cirq concrete implementation of this 2. You could imagine users might want a lower-touch way to put together an executable using an untyped, implicit-schema version of the spec. Ergo: this PR introduces `KeyValueExecutableSpec` to address these two concerns.
ExecutableSpecis a newly-introduced abstract class. The design was each application would subclass it for application-dependent data attributes.Ergo: this PR introduces
KeyValueExecutableSpecto address these two concerns.