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

feat: add syncing models utility to ivy #28818

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

YushaArif99
Copy link
Contributor

@YushaArif99 YushaArif99 commented Sep 12, 2024

Wanted to have your thoughts on how to expose these utility functions? I think it makes sense to have them inside the ivy.functional.backends.tensorflow.module.py module. But this creates an issue as to how we import this helper. So there wont be any ivy.sync_models_torch_and_tf but instead we'll have to do:

from ivy.functional.backends.tensorflow.module import sync_models_torch_and_tf
sync_models_torch_and_tf(...)

What do you guys suggest we do here @Sam-Armstrong @hmahmood24

@hmahmood24
Copy link
Contributor

Wanted to have your thoughts on how to expose these utility functions? I think it makes sense to have them inside the ivy.functional.backends.tensorflow.module.py module. But this creates an issue as to how we import this helper. So there wont be any ivy.sync_models_torch_and_tf but instead we'll have to do:

from ivy.functional.backends.tensorflow.module import sync_models_torch_and_tf
sync_models_torch_and_tf(...)

What do you guys suggest we do here @Sam-Armstrong @hmahmood24

My thinking was just exposing these as util functions so something like ivy.utils.sync_models_torch_and_tf or ivy.syncing_utils.sync_models_torch_and_tf seems cleaner to me rather than having to import the backends explicitly. What do you think @YushaArif99 ?

@Sam-Armstrong
Copy link
Contributor

@YushaArif99 Couldn't we just store all helper functions like this in a new file like ivy/utils/syncing.py or something like that? Is there any need to have this in the backend?

Also, I'd suggest we could expose a general ivy.sync_models function, which will detect the relevant models types and route to the correct function - sync_models_torch_and_tf or whatever. If anything other than torch and tf models are passed, for now we can just throw a ivy.exceptions.IvyNotImplementedException. I think this would be the best UX.

@YushaArif99
Copy link
Contributor Author

Sure! This is indeed a better UX. I was only inclined to not go this route as the logic contained within these utility helpers is framework-specific. And so it seems to deviate from the general convention of ivy API being an intermediary.
Then there's also a case of having dependency imports so will probably need to have the framework-specific imports as local imports.

Otherwise, this is definitely cleaner.

Based on this, should we still go forward with having these inside ivy.utils @hmahmood24 @Sam-Armstrong ?

@YushaArif99
Copy link
Contributor Author

@YushaArif99 Couldn't we just store all helper functions like this in a new file like ivy/utils/syncing.py or something like that? Is there any need to have this in the backend?

Also, I'd suggest we could expose a general ivy.sync_models function, which will detect the relevant models types and route to the correct function - sync_models_torch_and_tf or whatever. If anything other than torch and tf models are passed, for now we can just throw a ivy.exceptions.IvyNotImplementedException. I think this would be the best UX.

Yeah that's a good idea 👍🏼

@Sam-Armstrong
Copy link
Contributor

Sam-Armstrong commented Sep 12, 2024

@YushaArif99 I don't see an issue with it, because you'd have to import torch into the tensorflow backend anyway, which is also against the general convention of ivy. But I guess if we do expose ivy.sync_models it wouldn't be super important where functions like sync_models_torch_and_tf are located anyway, as they wouldn't be used directly by the user. So I guess it's up to you really 👍

@YushaArif99
Copy link
Contributor Author

@YushaArif99 I don't see an issue with it, because you'd have to import torch into the tensorflow backend anyway, which is also against the general convention of ivy. But I guess if we do expose ivy.sync_models it wouldn't be super important where functions like sync_models_torch_and_tf are located anyway, as they wouldn't be used directly by the user. So I guess it's up to you really 👍

Yeah I think exposing ivy.sync_models as the main interface is much cleaner. This way, we can have sync_models_torch_and_tf inside the TF module.py and similarily sync_models_torch_and_jax inside the JAX module.py which I feel is more intuitive.

I'll go ahead with this approach then. Thanks for the suggestions both!

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

Successfully merging this pull request may close these issues.

3 participants