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

Bad overload order for zeros_like #19685

Open
suo opened this issue Apr 24, 2019 · 5 comments
Open

Bad overload order for zeros_like #19685

suo opened this issue Apr 24, 2019 · 5 comments
Labels
module: pybind Related to our Python bindings / interactions with other Python libraries module: tensor creation triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@suo
Copy link
Member

suo commented Apr 24, 2019

In the pytorch_torch_functions.cpp generated code, the zeros_like binding looks like

static PyObject * THPVariable_zeros_like(PyObject* self_, PyObject* args, PyObject* kwargs)
{
  HANDLE_TH_ERRORS
  static PythonArgParser parser({
    "zeros_like(Tensor input, *, ScalarType dtype=None, Layout layout=torch.strided, Device device=None, bool pin_memory=False, bool requires_grad=False)",
    "zeros_like(Tensor input, *, bool requires_grad=False)",
  }, /*traceable=*/true);
...

In this case, the second overload will never get hit as all the arguments in the first overload have defaults. This is causing a bug where tracing will "constant-ify" device instead of correctly deriving the device at runtime from the input tensor (see #19637).

I tracked the overload sorting code to here but I don't understand it. My desired behavior is to swap the order of the overloads in cases like the above.

Can anyone help me out? cc. @gchanan, @cpuhrsch, @ezyang

@colesbury
Copy link
Member

Why are there two overloads in the first place? We should strive for only one overload as this makes error messages much more consistent with other Python functions.

@suo
Copy link
Member Author

suo commented Apr 24, 2019

That's true. In reality, all the TensorOptions arguments need to be optionals, and we should merge the TensorOptions passed with the input tensor's option as the fallback value.

Right now the logic to do that is in the binding code (pytorch_torch_functions), which means that it is not reusable from the JIT, so we would need to move that to the actual operator implementation.

Who is the right person to work on this @gchanan? I can help but I don't know how it fits into the ongoing schema unification thing

@fmassa fmassa added high priority module: pybind Related to our Python bindings / interactions with other Python libraries topic: operator triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Apr 26, 2019
@gchanan
Copy link
Contributor

gchanan commented Apr 30, 2019

removed high priority because I think the JIT issue is real, but the specific overload issue doesn't seem serious, as pointed out by @colesbury.

Who is the right person to work on this @gchanan? I can help but I don't know how it fits into the ongoing schema unification thing

The "frontend" schema unification part is complete in that all function signatures in native match their corresponding signature in JIT. The backends are still super complicated, because they do things like splatting / reverse splatting TensorOptions for no good reason.

I believe @VitalyFedyunin has been looking a bit into TensorOptions sanity, but I don't know exactly what he has planned in the short term.

@VitalyFedyunin
Copy link
Contributor

I'm on hold with the TensorOptions while @smessmer working on the codegen unification, As it might introduce more complexity into the code base.

@bhosmer
Copy link
Contributor

bhosmer commented Dec 12, 2019

@izdeby let's check back on this after #30983 lands. cc @gchanan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: pybind Related to our Python bindings / interactions with other Python libraries module: tensor creation triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

No branches or pull requests

7 participants