Skip to content
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

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

Open
apockill opened this issue May 24, 2021 · 9 comments

Comments

@apockill
Copy link
Collaborator

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
Copy link
Collaborator Author

apockill commented May 24, 2021

def get_all_devices() -> List[str]:

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
Copy link
Collaborator

velovix commented May 26, 2021

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
Copy link
Collaborator

BryceBeagle commented May 26, 2021

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
Copy link
Collaborator Author

apockill commented May 26, 2021

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
Copy link
Collaborator

BryceBeagle commented May 26, 2021

My vote is for the dataclass 👍.

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
Copy link
Collaborator Author

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
Copy link
Collaborator

velovix commented May 26, 2021

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
Copy link
Collaborator

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

@velovix
Copy link
Collaborator

velovix commented May 26, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants