opencv / open_vision_capsules

A set of libraries for encapsulating smart vision algorithms
Other
130 stars 48 forks source link

Clean up `get_all_devices` api to clear up confusion over OpenVINO "GPU" and Tensorflow "GPU" #35

Open apockill opened 3 years ago

apockill commented 3 years ago

Now that some implementations of the OpenVisionCapsules spec are integrating support for loading OpenVINO models onto integrated GPU's, there's a problem of name collisions from the device_mapping.get_all_devices() function. Curreuntly it returns all devices discovered by Tensorflow and OpenVINO combined.

apockill commented 3 years ago

https://github.com/opencv/open_vision_capsules/blob/cbd8bd9a390f407f8971562eaa129ccbf352da67/vcap/vcap/device_mapping.py#L13

Instead of returning List[str] it could return List[Tuple[DeviceProvider, str]], where DeviceProvider is an Enum, with values TENSORFLOW and OPENVINO, and perhaps others in the future.

I'm also open to major overhauls, if anyone has any clever ideas.

velovix commented 3 years ago

If it's an enum, would that prevent users from implementing their own DeviceMappers that map to their own hardware, since they can't add to the enum at runtime? I'm not sure if that's something anyone could do in practice though.

BryceBeagle commented 3 years ago

it could return List[DeviceProvider, str]

typing.List only supports homogeneous types (i.e. takes only one argument). Perhaps you meant Dict[DeviceProvider, str]? But this would limit you to only two devices, so maybe Dict[DeviceProvider, Set[str]] would be better?

apockill commented 3 years ago

That was a typo, oops! I actually meant List[Tuple[DeviceProvider, str]], because the filterer would likely want to iterate over the provider/device_name pairings. (Edited original post)

The current device mapper filters expect a list of device_names, and the workflow is to just iterate and filter anything that isn't compatible and return the compatible devices.

Another option would be to make a DeviceType dataclass, which has a Provider parameter, so basically:


class Provider(Enum)
    TENSORFLOW = "tensorflow"
    OPENVINO = "openvino"
    ...
    future providers

@dataclass
class DeviceType:
    provider: Provider
    name: str

def get_all_devices() -> List[DeviceType]:
    ...

Thoughts on this?

BryceBeagle commented 3 years ago

My vote is for the dataclass :+1:.

Could (should?) get_all_devices return Set[DeviceType]? (you'd have to mark DeviceType as frozen so it can be hashed). Does the order matter?

apockill commented 3 years ago

Technically get_all_devices operates with sets internally, then returns a list. So yes, it could and probably should return a set.

Order doesn't matter.

velovix commented 3 years ago

Alex and I talked about a possible design for this where vcap-utils creates Provider classes for TensorFlow and OpenVINO, then registers them with vcap under a string name. Then, the list of devices would be keyed by that string name. This allows other applications, including BrainFrame, to implement their own custom Providers and let capsules depend on them.

BryceBeagle commented 3 years ago

Why key them by string name instead of something higher-level?

velovix commented 3 years ago

Something higher level would be fine, I just don't think it should be an enum because that would prevent application developers from adding their own providers.