Add models for new server I/O #1123
Conversation
| @@ -0,0 +1,291 @@ | |||
| """Data models used when communicating with the superstaq server.""" | |||
|
|
|||
| # pragma: no cover | |||
There was a problem hiding this comment.
This will need to be removed once client-side starts using these models.
There was a problem hiding this comment.
Remind me why these aren't server side for now? https://github.com/Infleqtion/client-superstaq/pull/1106/files doesn't use these models either.
There was a problem hiding this comment.
https://github.com/Infleqtion/client-superstaq/pull/1106/files does use the models when handling server responses (it validates that the response is in the expected format). See for instance gss.client._SuperstaqClient_v0_3_0.create_job()`
I'm adding them to client side now so that I can import them server side.
There was a problem hiding this comment.
I see. Why is coverage being ignore for _model.py in that PR then?
There was a problem hiding this comment.
Because for now there are no tests that use any of these models (because they are just data structures)
| target: str | ||
| circuits: str | ||
| circuit_type: CircuitType | ||
| compiled_circuits: str | None = pydantic.Field(default=None) |
There was a problem hiding this comment.
does this not work?
| compiled_circuits: str | None = pydantic.Field(default=None) | |
| compiled_circuits: str | None = None |
There was a problem hiding this comment.
I think this is needed server-side to play nicely with fastapi.
| final_logical_to_physicals: list[str | None] | ||
|
|
||
|
|
||
| class NewJob(DefaultPydanticModel): |
There was a problem hiding this comment.
is this intended for jobs sent from the user to superstaq? if so why does it require both compiled and uncompiled circuits?
There was a problem hiding this comment.
This is intended to replace the "verbatim=True" option. On the server, if the compiled_circuits is provided then the server skips the compiling step
There was a problem hiding this comment.
ah ok. imo it would be nicer to just include the verbatim flag and a single circuits field, unless there is some case in which we'd need both
| ] | ||
|
|
||
|
|
||
| class DefaultPydanticModel(pydantic.BaseModel, use_enum_values=True, extra="forbid"): |
There was a problem hiding this comment.
we might want to rethink extra="forbid" - it'd be nice to add new fields server-side and not have it break existing clients
There was a problem hiding this comment.
My feeling is that for ease of use, self-documentation and secure coding, any I/O models should be fully enumerated. I agree it's a little less flexible though
There was a problem hiding this comment.
From dev meeting: Set extra=ignore for now.
| dry_run: bool = pydantic.Field(default=False) | ||
| sim_method: SimMethod | None = pydantic.Field(default=None, validate_default=True) | ||
| priority: int = pydantic.Field(default=0) | ||
| options_dict: dict[str, Any] = pydantic.Field(default={}) |
There was a problem hiding this comment.
this might be a good time to think about retiring the options dict. it was originally mostly intended as a stopgap so that we could make things flexible as we refined our interfaces, now that we're defining these models we should maybe to start breaking things into their own fields/models (and maybe just use for extra="allow" for experimental/wip/non-publicized fields?)
(not sure how we want to organize things though - we could put everything in the top level model, or keep a single options field w/ a model for possible arguments, or start separating e.g. compilation options/user credentials/simulation parameters into their own models...
There was a problem hiding this comment.
This is top of my to-do list once the initial server is up and running. As per my comment above about extra="forbid", the client side models should be fully enumerated.
I imagine that the best approach will be to have different models for different types of jobs etc., set up in some hierarchical way this will ensure that everything gets validated automatically by the server. This is part of the job refactor to-do list
For now I suggest we keep the options_dict field to maintain compatibility with existing Superstaq method.
There was a problem hiding this comment.
yep totally agree, just wanted to flag it in case it had any implications for how we go about organizing things
| """Target is simulator""" | ||
|
|
||
|
|
||
| class GetTargetsFilterModel(DefaultPydanticModel): |
There was a problem hiding this comment.
could this and the above model be combined?
There was a problem hiding this comment.
Although the field names are the same they represent different things so I feel they should remain as separate models. Also on server side some logic is added to the to the FilterModel
Co-authored-by: richrines1 <85512171+richrines1@users.noreply.github.com>
| ] | ||
|
|
||
|
|
||
| class DefaultPydanticModel(pydantic.BaseModel, use_enum_values=True, extra="forbid"): |
There was a problem hiding this comment.
two other options that are nice imo: validate_assignment=True and validate_default=True
…nt-superstaq into feature/v0.3.0_io_models
Adds data models that will be needed for v0.3.0 of the superstaq server --------- Co-authored-by: richrines1 <85512171+richrines1@users.noreply.github.com>
Adds data models that will be needed for v0.3.0 of the superstaq server