-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add Core functions for devices, properties, version #114
Conversation
Co-authored-by: Bradley Odell <[email protected]>
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.
I think this change is good functionality-wise. I made some style comments to address but otherwise it is good to go.
crates/openvino/src/core.rs
Outdated
/// Gets device plugins version information. | ||
/// Device name can be complex and identify multiple devices at once like `HETERO:CPU,GPU`; | ||
/// in this case, the returned map contains multiple entries, each per device. | ||
pub fn versions(&self, device_name: impl AsRef<str>) -> Result<HashMap<DeviceType, Version>> { |
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.
Let's just return a Result<Vec<(DeviceType, Version)>>
; if users really need a map there are conversion functions for that. And they might want some other collection besides a hash map so let's let give them the most simple data structure.
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.
Done
crates/openvino/src/core.rs
Outdated
|
||
/// Gets properties related to this Core. | ||
/// The method extracts information that can be set via the [set_property] method. | ||
pub fn property(&self, key: PropertyKey) -> Result<Cow<str>> { |
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.
Maybe get_property
for consistency?
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.
Done
crates/openvino/src/core.rs
Outdated
|
||
/// Gets properties related to device behaviour. | ||
/// The method extracts information that can be set via the [set_device_property] method. | ||
pub fn device_property( |
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.
pub fn device_property( | |
pub fn get_device_property( |
?
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.
Done
crates/openvino/src/core.rs
Outdated
&self, | ||
device_name: impl AsRef<str>, | ||
key: PropertyKey, | ||
) -> Result<Cow<str>> { |
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.
Hm, I'm not sure about this Cow... You're trying to avoid a copy?
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.
I have removed the Cow
from return. I was using to_string_lossy
which returned Cow<str>
, handling non-utf8 and utf-8 cases both, so returning Cow
would save copy for valid utf8.
I changed that now to to_str
.
crates/openvino/src/core.rs
Outdated
/// Sets a property for a device. | ||
pub fn set_device_property( | ||
&mut self, | ||
device_name: impl AsRef<str>, |
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.
Why the AsRef
here but not for value
? Just a consistency nit, but it may be worthwhile to think through why we would use the trait versus the simpler &str
. Either is fine though I try to bias towards simpler.
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.
Done, I removed AsRef
s
} | ||
|
||
/// Sets properties for a device. | ||
pub fn set_device_properties<'a>( |
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.
Is the 'a
still needed?
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.
Yes I get an error otherwise, anonymous lifetimes in impl Trait are unstable
Add functions to core related to devices and properties.