Skip to content

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented May 8, 2020

Stack from ghstack:

This removes the error prone process of assembling torch/__init__.pyi
(and frequently forgetting to expose things), since now we can simply
rely on the true source file to get things done. Most of the old
codegen in gen_pyi.py is now rerouted to various files:

  • torch/_C/__init__.pyi (the dumping pile of all misc bindings)
  • torch/_C/_nn.pyi (NN function bindings)
  • torch/_C/_VariableFunctions.pyi (torch function bindings)

torch.types grew a bunch more definitions that previously where
defined in torch/__init__.pyi

Some miscellaneous changes

  • Fixed a bug where we treat single TensorList argument as implying
    varargs are accepted. This is actually only supported on IntList.
    This means we can correctly generate a stub for dequantize.
  • Add missing manual stub for nonzero
  • Switched torch/onnx/operators.py to directly refer to _C module,
    since apparently mypy doesn't think that methods prefixed with
    underscores get reexported. This may be a recurring theme; maybe
    we need to find a better way to solve it.

Because I was really lazy, I dumped namedtuple definitions in both
torch._C and torch._C._VariableFunctions. This is definitely wrong.

Signed-off-by: Edward Z. Yang ezyang@fb.com

Differential Revision: D21497400

This removes the error prone process of assembling torch/__init__.pyi
(and frequently forgetting to expose things), since now we can simply
rely on the true source file to get things done.  Most of the old
codegen in gen_pyi.py is now rerouted to various files:

- torch/_C/__init__.pyi (the dumping pile of all misc bindings)
- torch/_C/_nn.pyi (NN function bindings)
- torch/_C/_VariableFunctions.pyi (torch function bindings)

torch.types grew a bunch more definitions that previously where
defined in torch/__init__.pyi

Some miscellaneous changes

- Fixed a bug where we treat single TensorList argument as implying
  varargs are accepted. This is actually only supported on IntList.
  This means we can correctly generate a stub for dequantize.
- Add missing manual stub for nonzero
- Switched torch/onnx/operators.py to directly refer to _C module,
  since apparently mypy doesn't think that methods prefixed with
  underscores get reexported.  This may be a recurring theme; maybe
  we need to find a better way to solve it.

Because I was really lazy, I dumped namedtuple definitions in both
torch._C and torch._C._VariableFunctions.  This is definitely wrong.

Signed-off-by: Edward Z. Yang <ezyang@fb.com>

[ghstack-poisoned]
@ezyang ezyang requested a review from apaszke as a code owner May 8, 2020 22:01
ezyang added a commit that referenced this pull request May 8, 2020
This removes the error prone process of assembling torch/__init__.pyi
(and frequently forgetting to expose things), since now we can simply
rely on the true source file to get things done.  Most of the old
codegen in gen_pyi.py is now rerouted to various files:

- torch/_C/__init__.pyi (the dumping pile of all misc bindings)
- torch/_C/_nn.pyi (NN function bindings)
- torch/_C/_VariableFunctions.pyi (torch function bindings)

torch.types grew a bunch more definitions that previously where
defined in torch/__init__.pyi

Some miscellaneous changes

- Fixed a bug where we treat single TensorList argument as implying
  varargs are accepted. This is actually only supported on IntList.
  This means we can correctly generate a stub for dequantize.
- Add missing manual stub for nonzero
- Switched torch/onnx/operators.py to directly refer to _C module,
  since apparently mypy doesn't think that methods prefixed with
  underscores get reexported.  This may be a recurring theme; maybe
  we need to find a better way to solve it.

Because I was really lazy, I dumped namedtuple definitions in both
torch._C and torch._C._VariableFunctions.  This is definitely wrong.

Signed-off-by: Edward Z. Yang <ezyang@fb.com>

ghstack-source-id: 7c33437
Pull Request resolved: #38157
@ezyang ezyang requested a review from rgommers May 8, 2020 22:45
@dr-ci
Copy link

dr-ci bot commented May 8, 2020

💊 CI failures summary and remediations

As of commit 6fac1e1 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 10 times.

…tubs"

This removes the error prone process of assembling torch/__init__.pyi
(and frequently forgetting to expose things), since now we can simply
rely on the true source file to get things done.  Most of the old
codegen in gen_pyi.py is now rerouted to various files:

- torch/_C/__init__.pyi (the dumping pile of all misc bindings)
- torch/_C/_nn.pyi (NN function bindings)
- torch/_C/_VariableFunctions.pyi (torch function bindings)

torch.types grew a bunch more definitions that previously where
defined in torch/__init__.pyi

Some miscellaneous changes

- Fixed a bug where we treat single TensorList argument as implying
  varargs are accepted. This is actually only supported on IntList.
  This means we can correctly generate a stub for dequantize.
- Add missing manual stub for nonzero
- Switched torch/onnx/operators.py to directly refer to _C module,
  since apparently mypy doesn't think that methods prefixed with
  underscores get reexported.  This may be a recurring theme; maybe
  we need to find a better way to solve it.

Because I was really lazy, I dumped namedtuple definitions in both
torch._C and torch._C._VariableFunctions.  This is definitely wrong.

Signed-off-by: Edward Z. Yang <ezyang@fb.com>

[ghstack-poisoned]
ezyang added a commit that referenced this pull request May 9, 2020
This removes the error prone process of assembling torch/__init__.pyi
(and frequently forgetting to expose things), since now we can simply
rely on the true source file to get things done.  Most of the old
codegen in gen_pyi.py is now rerouted to various files:

- torch/_C/__init__.pyi (the dumping pile of all misc bindings)
- torch/_C/_nn.pyi (NN function bindings)
- torch/_C/_VariableFunctions.pyi (torch function bindings)

torch.types grew a bunch more definitions that previously where
defined in torch/__init__.pyi

Some miscellaneous changes

- Fixed a bug where we treat single TensorList argument as implying
  varargs are accepted. This is actually only supported on IntList.
  This means we can correctly generate a stub for dequantize.
- Add missing manual stub for nonzero
- Switched torch/onnx/operators.py to directly refer to _C module,
  since apparently mypy doesn't think that methods prefixed with
  underscores get reexported.  This may be a recurring theme; maybe
  we need to find a better way to solve it.

Because I was really lazy, I dumped namedtuple definitions in both
torch._C and torch._C._VariableFunctions.  This is definitely wrong.

Signed-off-by: Edward Z. Yang <ezyang@fb.com>

ghstack-source-id: 300b9f6
Pull Request resolved: #38157
@rgommers rgommers added the module: typing Related to mypy type annotations label May 9, 2020
_dtype = torch.dtype
_device = torch.device
_qscheme = torch.qscheme
_size = Union[torch.Size, List[_int], Tuple[_int, ...]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little inconsistent compared with all the other underscored names above, one would expect _size to mean torch.size.

The generated _C/__init__.pyi now has a number of signatures with:

self, size: _size, *, dtype: _dtype=None, layout: _layout=strided, device: Union[_device, str, None]=None,

I think it would make sense to change _device above to _device: Union[torch.device, str, None] so the signatures become:

self, size: _size, *, dtype: _dtype=None, layout: _layout=strided, device: _device=None, ...

And if people need to use device rather than that union to annotate a device parameter, they can always use the explicit torch.device, the fully-qualified name should avoid mypy/issues/4146.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to note is that these names are all grandfathered from the old type stubs. So while I'm generally amenable to changing things up, it will greatly increase the surface area of the patch.

@rgommers
Copy link
Collaborator

rgommers commented May 9, 2020

Hmm, CI passed but I'm getting a number of errors locally like:

torch/_C/__init__.pyi:741: error: Incompatible default for argument "layout" (default has type "torch._C.layout", argument has type "torch.layout")  [assignment]

I'll do a full rebuild, maybe some codegen doesn't get triggered correctly on a partial rebuild.

# top-level values. The underscore variants let us refer to these
# types. See https://github.com/python/mypy/issues/4146 for why these
# workarounds is necessary
_int = builtins.int
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is not really a mypy bug, the issue is that torch.int exists and means torch.int32 rather than builtins.int. Given that torch.types cannot be used in the main torch namespace (have to avoid an import cycle), the better approach here would be that there's no shadowing in any other modules, so one can use plain int there.

Copy link
Collaborator

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth writing down the rules for what goes into torch.types, and making that a consistent design. Now for types._name, name can refer to a builtin, something in the torch namespace, or a useful Union defined inside types.

I'd lean towards being conservative for now, and only define unions here. The aliasing problem can be avoided by being careful in other files, and using fully-qualified names like torch.layout.

@rgommers
Copy link
Collaborator

rgommers commented May 9, 2020

@ezyang and I discussed offline: let's merge types.py as is, and worry about cleaning up the public API of that before the splitoff of the next release branch, to be able to move faster now and not make these initial PRs really large.

…tubs"


This removes the error prone process of assembling `torch/__init__.pyi`
(and frequently forgetting to expose things), since now we can simply
rely on the true source file to get things done.  Most of the old
codegen in gen_pyi.py is now rerouted to various files:

- `torch/_C/__init__.pyi` (the dumping pile of all misc bindings)
- `torch/_C/_nn.pyi` (NN function bindings)
- `torch/_C/_VariableFunctions.pyi` (torch function bindings)

`torch.types` grew a bunch more definitions that previously where
defined in `torch/__init__.pyi`

Some miscellaneous changes

- Fixed a bug where we treat single TensorList argument as implying
  varargs are accepted. This is actually only supported on IntList.
  This means we can correctly generate a stub for dequantize.
- Add missing manual stub for nonzero
- Switched torch/onnx/operators.py to directly refer to _C module,
  since apparently mypy doesn't think that methods prefixed with
  underscores get reexported.  This may be a recurring theme; maybe
  we need to find a better way to solve it.

Because I was really lazy, I dumped namedtuple definitions in both
`torch._C` and `torch._C._VariableFunctions`.  This is definitely wrong.

Signed-off-by: Edward Z. Yang <ezyang@fb.com>

[ghstack-poisoned]
…tubs"


This removes the error prone process of assembling `torch/__init__.pyi`
(and frequently forgetting to expose things), since now we can simply
rely on the true source file to get things done.  Most of the old
codegen in gen_pyi.py is now rerouted to various files:

- `torch/_C/__init__.pyi` (the dumping pile of all misc bindings)
- `torch/_C/_nn.pyi` (NN function bindings)
- `torch/_C/_VariableFunctions.pyi` (torch function bindings)

`torch.types` grew a bunch more definitions that previously where
defined in `torch/__init__.pyi`

Some miscellaneous changes

- Fixed a bug where we treat single TensorList argument as implying
  varargs are accepted. This is actually only supported on IntList.
  This means we can correctly generate a stub for dequantize.
- Add missing manual stub for nonzero
- Switched torch/onnx/operators.py to directly refer to _C module,
  since apparently mypy doesn't think that methods prefixed with
  underscores get reexported.  This may be a recurring theme; maybe
  we need to find a better way to solve it.

Because I was really lazy, I dumped namedtuple definitions in both
`torch._C` and `torch._C._VariableFunctions`.  This is definitely wrong.

Signed-off-by: Edward Z. Yang <ezyang@fb.com>

Differential Revision: [D21497400](https://our.internmc.facebook.com/intern/diff/D21497400)

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in 6edf340.

@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in 6edf340.

@facebook-github-bot facebook-github-bot deleted the gh/ezyang/751/head branch May 15, 2020 14:18
andyljones added a commit to andyljones/pytorch that referenced this pull request Aug 25, 2020
PR pytorch#38157 fixed type checking for mypy by including `if False` guards
on some type-checker-only imports. However other typecheckers - like
pyright - will respect this logic and ignore the imports. Using
the TYPE_CHECKING var instead means both mypy and pyright will work.
facebook-github-bot pushed a commit that referenced this pull request Sep 3, 2020
Summary:
PR #38157 fixed type checking for mypy by including `if False` guards on some type-checker-only imports. However other typecheckers - [like pyright](microsoft/pylance-release#262 (comment)) - will respect this logic and ignore the imports. Using [`if TYPE_CHECKING`](https://docs.python.org/3/library/typing.html#typing.TYPE_CHECKING) instead means both mypy and pyright will work correctly.

[For background, an example of where the current code fails](microsoft/pylance-release#262) is if you make a file `tmp.py` with the contents
```python
import torch
torch.ones((1,))
```
Then [`pyright tmp.py --lib`](https://github.com/microsoft/pyright#command-line) will fail with a `"ones" is not a known member of module` error. This is because it can't find the `_VariableFunctions.pyi` stub file, as pyright respects the `if False` logic. After adding the `TYPE_CHECKING` guard, all works correctly.

Credit to erictraut for suggesting the fix.

Pull Request resolved: #43339

Reviewed By: agolynski

Differential Revision: D23348142

Pulled By: ezyang

fbshipit-source-id: c8a58122a7b0016845c311da39a1cc48748ba03f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: typing Related to mypy type annotations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants