[JIT] Fix type annotations of pooling modules#47888
[JIT] Fix type annotations of pooling modules#47888eellison wants to merge 4 commits intogh/eellison/135/basefrom
Conversation
[ghstack-poisoned]
Fix for #45904 All of the pooling modules except MaxUnpool and LPPool return either a Tensor or a tuple of [Tensor, Tensor]. The current type annotations are inaccurate, and prevent scripting the module if `return_indices` is set as true in the module. [ghstack-poisoned]
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 18ffc48 (more details on the Dr. CI page): Commit 18ffc48 was recently pushed. Waiting for builds... 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 to the (internal) Dr. CI Users group. |
ezyang
left a comment
There was a problem hiding this comment.
This leaves me a bad taste in my mouth, but that isn't any reason to block landing this particular PR. What is really irritating, though, is that I can't do the less useful type Union[Tensor, Tuple[...]] because JIT will choke on it. There still ought to be a way to mark types to be ignored by JIT.
|
Someone will run into this again when removing the Adding a comment above the first
I can't find an existing issue for that so quickly, do you know if there is one? |
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
|
This is about ready to land, modulo adding a comment and a rebase. Could this be landed before the bot closes the PR? |
Fix for #45904 All of the pooling modules except MaxUnpool and LPPool return either a `Tensor `or `[Tensor, Tensor]`. The current type annotations are inaccurate, and prevent scripting the module if `return_indices` is set as true in the module. There's not a great way to make this agree with mypy because the overload is dependent on the value of `return_indices`, an attribute. [ghstack-poisoned]
Fix for #45904 All of the pooling modules except MaxUnpool and LPPool return either a `Tensor `or `[Tensor, Tensor]`. The current type annotations are inaccurate, and prevent scripting the module if `return_indices` is set as true in the module. There's not a great way to make this agree with mypy because the overload is dependent on the value of `return_indices`, an attribute. [ghstack-poisoned]
|
Cleaning my github dashboard, doesn't seem this PR is needed due to inactivity, removing myself as reviewer. |
|
@eellison any reason this hasn't been merged yet? |
|
I rebased this and fixed some issues in tests: #65847 If a FB employee would please review and import I'd appreciate it. |
|
@eellison is an fb employee 😂 |
|
Hi @garymm i moved projects and this sort of slipped out under me. as you see, it's pretty close, if you don't mind rebasing/getting CI to pass i'll review and land, thanks for your help! |
|
Closing this in favor of #65847 |
Stack from ghstack:
Fix for #45904
All of the pooling modules except MaxUnpool and LPPool return either a
Tensoror[Tensor, Tensor]. The current type annotations are inaccurate, and prevent scripting the module ifreturn_indicesis set as true in the module.There's not a great way to make this agree with mypy because the overload is dependent on the value of
return_indices, an attribute.