-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
fix(framework): refactoring as_ivy_dev and as_native_dev in all backends #21269
Conversation
Thanks for contributing to Ivy! 😊👏 |
If you are working on an open task, please edit the PR description to link to the issue you've created. For more information, please check ToDo List Issues Guide. Thank you 🤗 |
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.
Hey @abdulasiraj, thanks for looking into this task, I think we could just use as_native_dev
inside as_ivy_dev
to avoid duplicating the fallback logic.
Happy to know your thoughts 😄
Hey @vedpatwardhan, |
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.
Thanks for clarifying @abdulasiraj
I think what we should do in that case is get the common logic between these 2 functions (as_ivy_dev
and as_native_dev
) as a private helper which both of these functions would make use of, happy to know what you think 😄
Yep, It'll be a good solution. let me try! |
Hi @vedpatwardhan, |
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.
Hi @vedpatwardhan, I created the private functions where possible but It's not possible for most of the time!
Actually in your PR, in the backends/jax/device.py
for e.g., except for the ivy.Device
calls in as_ivy_dev
, rest of the code is the same as that in as_native_dev
. We could avoid this code duplication through a single helper that executes this common functionality 😄
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.
The creation of ivy.Device object is repeated multiple times. This can be minimized.
Hi @vedpatwardhan, It's possible to add helper for torch and jax but for paddle it's not yet possible cuz paddle missing some functions. I've raised issue on paddlepaddle repo. |
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.
Hey @abdulasiraj, thanks for adding the helper for the jax
and torch
backends and also creating the issue on the paddle repo, just a few more suggestions,
dev_helper
should be private, i.e._dev_helper
.- Probably we can reduce the duplication further, if you see the
dev_helper
forjax
andtorch
, there isn't a lot of difference except for usingjax.devices
forjax
andtorch.device
fortorch
. So we could in fact, move the_dev_helper
tofunctional/ivy/device.py
which accepts anative_device_fn
argument and then we'll import this helper into our backenddevices.py
and pass the correctnative_device_fn
fortorch
andjax
. - Could you please also bring your branch up to date with
main
? There were quite a few issues with the testing pipeline before, which have been fixed now, so updating your branch to be up to date withmain
should reduce the number of failures in the CI.
Happy to know what you think, thanks @abdulasiraj 😄
This PR has been labelled as stale because it has been inactive for more than 7 days. If you would like to continue working on this PR, then please add another comment or this PR will be closed in 7 days. |
Hey @abdulasiraj, are you creating a different PR for this issue? |
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.
PR Compliance Checks
Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.
Issue Reference
In order to be considered for merging, the pull request description must refer to a specific issue number. This is described in our contributing guide and our PR template.
This check is looking for a phrase similar to: "Fixes #XYZ" or "Resolves #XYZ" where XYZ is the issue number that this PR is meant to address.
Hey @vedpatwardhan, |
Got it! If you're busy with something else, @RickSanchezStoic could you please look into getting this over the line? I think we're done for the most part just a final few changes remaining. Thanks 😄 |
dbe802b
to
7a1b4ed
Compare
I'll create new PR for this |
Thank you for this PR, here is the CI results: Failed tests:This PR introduces the following new failing tests: |
Thank you for this PR, here is the CI results: Failed tests:This PR introduces the following new failing tests: |
will also close, #18247