-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat(python): support TranslationOptions #307
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,45 @@ | ||||||||
from typing import Optional, final | ||||||||
|
||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
class TranslationOptions: | ||||||||
translation_backend: Optional[TranslationBackend] = None | ||||||||
|
||||||||
... | ||||||||
|
||||||||
@final | ||||||||
class TranslationBackend: | ||||||||
""" | ||||||||
Object specifying which translation backend to use to translate a particular program, | ||||||||
and for that given backend, what options to apply. | ||||||||
|
||||||||
Variants: | ||||||||
``v1``: Corresponds to the V1 translation backend. | ||||||||
``v2``: Corresponds to the V2 translation backend. | ||||||||
|
||||||||
Methods (each per variant): | ||||||||
- ``is_*``: if the underlying values are that type. | ||||||||
- ``as_*``: if the underlying values are that type, then those values, otherwise ``None``. | ||||||||
- ``to_*``: the underlying values as that type, raises ``ValueError`` if they are not. | ||||||||
- ``from_*``: wrap underlying values as this enum type. | ||||||||
|
||||||||
""" | ||||||||
|
||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
def is_v1(self) -> bool: ... | ||||||||
def is_v2(self) -> bool: ... | ||||||||
def as_v1(self) -> Optional[BackendV1Options]: ... | ||||||||
def as_v2(self) -> Optional[BackendV2Options]: ... | ||||||||
def to_v1(self) -> BackendV1Options: ... | ||||||||
def to_v2(self) -> BackendV2Options: ... | ||||||||
@staticmethod | ||||||||
def from_v1(inner: BackendV1Options) -> "TranslationBackend": ... | ||||||||
@staticmethod | ||||||||
def from_v2(inner: BackendV2Options) -> "TranslationBackend": ... | ||||||||
|
||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
class BackendV1Options: | ||||||||
""" | ||||||||
Options for the V1 translation backend. | ||||||||
""" | ||||||||
|
||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
class BackendV2Options: | ||||||||
""" | ||||||||
Options for the V2 translation backend. | ||||||||
""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,9 @@ | ||
use rigetti_pyo3::create_init_submodule; | ||
|
||
pub mod models; | ||
|
||
create_init_submodule! { | ||
submodules: [ | ||
"models": models::init_submodule | ||
], | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,10 @@ | ||
use rigetti_pyo3::create_init_submodule; | ||
|
||
pub mod controller; | ||
pub mod translation; | ||
|
||
create_init_submodule! { | ||
submodules: [ | ||
"translation": translation::init_submodule | ||
], | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,18 +1,37 @@ | ||||||
use qcs_api_client_grpc::services::translation::{ | ||||||
translation_options::TranslationBackend, BackendV1Options, BackendV2Options, TranslationOptions, | ||||||
}; | ||||||
use rigetti_pyo3::{py_wrap_data_struct, py_wrap_type, py_wrap_union_enum}; | ||||||
use rigetti_pyo3::{ | ||||||
create_init_submodule, py_wrap_data_struct, py_wrap_type, py_wrap_union_enum, | ||||||
pyo3::{pymethods, PyResult}, | ||||||
}; | ||||||
|
||||||
py_wrap_type! { | ||||||
#[derive(Default)] | ||||||
PyBackendV1Options(BackendV1Options) as "BackendV1Options"; | ||||||
} | ||||||
|
||||||
#[pymethods] | ||||||
impl PyBackendV1Options { | ||||||
#[new] | ||||||
fn __new__() -> PyResult<Self> { | ||||||
Ok(Self::default()) | ||||||
} | ||||||
} | ||||||
|
||||||
py_wrap_type! { | ||||||
#[derive(Default)] | ||||||
PyBackendV2Options(BackendV2Options) as "BackendV2Options"; | ||||||
} | ||||||
|
||||||
#[pymethods] | ||||||
impl PyBackendV2Options { | ||||||
#[new] | ||||||
fn __new__() -> PyResult<Self> { | ||||||
Ok(Self::default()) | ||||||
} | ||||||
} | ||||||
|
||||||
py_wrap_union_enum! { | ||||||
PyTranslationBackend(TranslationBackend) as "TranslationBackend" { | ||||||
v1: V1 => PyBackendV1Options, | ||||||
|
@@ -26,3 +45,22 @@ py_wrap_data_struct! { | |||||
translation_backend: Option<TranslationBackend> => Option<PyTranslationBackend> | ||||||
} | ||||||
} | ||||||
|
||||||
#[pymethods] | ||||||
impl PyTranslationOptions { | ||||||
#[new] | ||||||
fn __new__(translation_backend: Option<PyTranslationBackend>) -> PyResult<Self> { | ||||||
Ok(Self(TranslationOptions { | ||||||
translation_backend: translation_backend.map(|b| b.0), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typically, we use the
Suggested change
|
||||||
})) | ||||||
} | ||||||
} | ||||||
|
||||||
create_init_submodule! { | ||||||
classes: [ | ||||||
PyTranslationBackend, | ||||||
PyTranslationOptions, | ||||||
PyBackendV1Options, | ||||||
PyBackendV2Options | ||||||
], | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,7 @@ create_init_submodule! { | |
submodules: [ | ||
"client": client::init_submodule, | ||
"compiler": compiler::init_submodule, | ||
"grpc": grpc::init_submodule, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know you are just carrying forward with an existing module and just actually exporting it here, and I know the plan is to add a wrapped type later, but I don't love exporting raw bindings to the API here, and especially exposing them as a "GRPC" module to users. Other modules get coupled to it because by it's very nature it contains models that requests across the stack need, but the individual models are typically scoped to a single kind of request, so why have them all co-located? It's also ambiguous when you need to pull it in, since our services use a mix of REST and GRPC. To me, it feels like we're exposing unnecessary implementation details to users and making it more complex than needed to reason about our API and how to use it. I could be convinced to re-export the needed GRPC model directly from the translation module, that way it is at least grouped within a concept, reducing the number of imports needed and making it easier to find the needed type with tooling. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Went in that direction on a new PR, #308 |
||
"qpu": qpu::init_submodule, | ||
"qvm": qvm::init_submodule | ||
], | ||
|
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 file is orphaned. The
grpc
andmodels
directory both need__init__.pyi
files exportingmodels
, andtranslation
, respectively.