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

vmap-incompatible inplace being used by existing functions #247

Open
Padarn opened this issue Nov 7, 2021 · 3 comments
Open

vmap-incompatible inplace being used by existing functions #247

Padarn opened this issue Nov 7, 2021 · 3 comments

Comments

@Padarn
Copy link
Contributor

Padarn commented Nov 7, 2021

Hi all,

I'm working on this PR #225, and am having some trouble fixing one of the tests which complains of

FAILED test/test_ops.py::TestOperatorsCPU::test_vmapvjp_clamp_cpu_float32 - RuntimeError: vmap: aten::logical_or_(self, *extra_args) is not possible because there exists a Tensor `other` in extra_args that has more elements than `self`. This happened due to `other` being vmapped over but `...

So I understand this error now, and where is it coming from: The backwards function for clamp ends up using logical_or_ in the way described. But, I don't understand what the ideal fix would be.

Some thoughts:

  • Change the backwards function for clamp so that it doesn't use the in place operation of logical_or?
  • Completely rewrite the batching rule for the backwards clamp so that it doesn't fallback into this path?

The second seems like a bad precedent, and the first seems a bit overkill.

So, any advice on how this kind of situation should be handled? Or is already handled by other parts of the codebase? (please feel free to point me to code rather than explaining)

@vfdev-5
Copy link
Contributor

vfdev-5 commented Nov 8, 2021

@Padarn I think there is an internal development on "functionalization" of ops to replace inplace ops with non-inplace ones. Please check out : #241 , #235

@Padarn
Copy link
Contributor Author

Padarn commented Nov 9, 2021

I see that makes, thanks @vfdev-5.

@zou3519 anything I can help with here, or best to just wait for the internal resolution of this?

@vfdev-5
Copy link
Contributor

vfdev-5 commented Nov 9, 2021

@Padarn you can take a look at #240 , I started from the top and you can tackle subtasks from the bottom

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

2 participants