-
Notifications
You must be signed in to change notification settings - Fork 6
Add UniFFI client with Python bindings #71
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
Conversation
… into lib to make python install easier, add install example docs, and clean up.
0c53279 to
aeb8202
Compare
Synicix
left a comment
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.
Some discussion points for next meeting
| pub fn make_path<T>(&self, hash: &str, relpath: impl AsRef<Path>) -> PathBuf { | ||
| pub fn make_path<T: fmt::Debug>( | ||
| &self, | ||
| model: &T, |
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.
What was the reason to change
pub fn make_path<T>(&self, hash: &str, relpath: impl AsRef<Path>) -> PathBufto include model: &T, when users of this function can still use
::<T>make_path()| clippy::unwrap_used, | ||
| reason = "Cannot return `None` since debug format always returns `String`." | ||
| )] | ||
| pub fn parse_debug_name<T: fmt::Debug>(instance: &T) -> String { |
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.
What was the reason behind adding this function when it does similar to what get_type_name does?
src/uniffi/model.rs
Outdated
|
|
||
| /// Available models. | ||
| #[derive(uniffi::Enum, Debug)] | ||
| pub enum Model { |
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.
Perhaps go with ModelType or Type?
| #[test] | ||
| fn consistent_hash() -> Result<()> { | ||
| let filepath = "./tests/data/images/subject.jpeg"; | ||
| let filepath = "./tests/extra/data/images/subject.jpeg"; |
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.
What was the reason to add extra?
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.
Perhaps later we should match against the kind to see if it returns the correct error type?
tests/extra/python/smoke_test.py
Outdated
| version="1.0.0", | ||
| ), | ||
| image="alpine:3.14", | ||
| # command=r"sh -c 'sleep 5 && echo finished...'", # needs arg parsing before will work |
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.
Stale comment code?
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Patch coverage here is less than 100% but I think for this particular case it is reasonable so leaving it as-is. It is because of UniFFI. Reasons:
The benefit doesn't seem worth it right now so I recommend we defer. |
Synicix
left a comment
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.
Reviewed with zoom meeting
Depends on #70, #74
UniFFI Note:
Objectis when something is Rust-owned and passed by reference to the client. Methods are supported but indexing is only supported via methods e.g. getters.Recordis when you transfer ownership to the client and passed by value. Methods are not supported but the client provides native indexing support.Enumis most similar to aRecordabove but for a Rust enum.In this PR, we:
uniffi-bindgenbinary directly in our package since it simplifies the orcapod installation steps (users don't need to install it separately).async-traitdependency soasync fnwork with dyn traits.getsetdependency which allows auto deriving getters for structs. Sourcing from my fork since needed to add a feature to propagate specified attributes (Addimpl_attrsfeature jbaublitz/getset#114).derive_more(display) to simplify how to modifyDisplayimplementations since there is good integration with UniFFI i.e. this is how you modify how a Rust-owned object is displayed on the client side.Object:Pod,PodJob,OrcaError,LocalDockerOrchestrator,LocalFileStore.Record:PodResult,Annotation,GPURequirement,StreamInfo,OrcaPath,Blob,RunInfo,PodRun,ModelInfo.Enum:Model,GPUModel,Input,BlobKind,ImageKind,Status,ModelID.Arcsince UniFFI requires this for anyObjectusage e.g.PodJob { pod: Pod, .. }->PodJob { pod: Arc<Pod>, .. }.delete_annotationto non-generic since this is not allowed across CFFI boundary. Used a variation based on enum.parse_debug_name: a similar method toget_type_namebut sourcing from the debug display. This is useful since it can be called with a struct or an enum.OrcaError(Kind)toOrcaError { kind: Kind}since UniFFI did not allow modifying display on tuple structs.MODEL_NAMESPACEconstant out of trait since this is not supported in UniFFI.pyproject.tomlso others can easily install from source e.g.pip install git+https://github.com/walkerlab/orcapod.extra/subdirectory.tests/extra/python/smoke_test.pyto verify some basic features in Python client. Supports using debugger.docs/examplesand move diagram todocs/images/..clippy.toml.min-ident-chars-threshold=1(default).allow-duplicate-cratesto current offenders