Rereapply optional ScalarType interface changes that were reverted in D16079809 #22456
Conversation
This reverts commit dff2c07.
pytorchbot
added
oncall: jit
wanchaol
left a comment
wanchaol
left a comment
There was a problem hiding this comment.
Can you add a test for log_softmax to test this specific positional argument/ kw-only argument problem? I would like to ensure we notice this breakage next time if we change the API again
|
FYI @ailzhang, when I re-land this it'll re-break XLA |
I think the test we'll want will be in jit? |
While I think we should add to the somewhere that JIT tests and Autograd tests can share with each other, like |
|
@nairbv we should add |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@nairbv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
I'm happy to add tests, but it seems like if someone is explicitly changing the API (as I was) they might just also change the tests. If we had a test that attempted to load a model and failed, it'd be much clearer that something was meaningfully incorrect. Asserting that an API is what the API says it is seems a bit redundant. |
|
@nairbv Yea I agree. We really should have had test coverage for those so that we knew it's BC breaking. I'm only requesting adding tests here to prevent future breakage :D |
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@nairbv is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@nairbv has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
… D16079809 (#22456) Summary: re-apply changes reverted in: pytorch/pytorch#22412 Also change log_softmax to take positional arguments. Long-term we do want the kwarg-only interface, but seems to currently be incompatible with jit serialization. Pull Request resolved: pytorch/pytorch#22456 Differential Revision: D16097159 Pulled By: nairbv fbshipit-source-id: 8cb73e9ca18fc66b35b873cf4a574b167a578b3d
| variants: function, method | ||
|
|
||
| - func: log_softmax(Tensor self, int dim) -> Tensor | ||
| - func: log_softmax(Tensor self, int dim, ScalarType? dtype=None) -> Tensor |
There was a problem hiding this comment.
we should comment why this and softmax don't follow the usual convention.
… D16079809 (pytorch#22456) Summary: re-apply changes reverted in: pytorch#22412 Also change log_softmax to take positional arguments. Long-term we do want the kwarg-only interface, but seems to currently be incompatible with jit serialization. Pull Request resolved: pytorch#22456 Differential Revision: D16097159 Pulled By: nairbv fbshipit-source-id: 8cb73e9ca18fc66b35b873cf4a574b167a578b3d
Summary: Address the review comment made by gchanan here: #22456 (comment) Pull Request resolved: #22651 Differential Revision: D16181828 Pulled By: nairbv fbshipit-source-id: 0d41a9024c2664298c281e198a997be73e7f8499
Summary: Address the review comment made by gchanan here: pytorch/pytorch#22456 (comment) Pull Request resolved: pytorch/pytorch#22651 Differential Revision: D16181828 Pulled By: nairbv fbshipit-source-id: 0d41a9024c2664298c281e198a997be73e7f8499
7 participants
re-apply changes reverted in:
#22412
Also change log_softmax to take positional arguments. Long-term we do want the kwarg-only interface, but seems to currently be incompatible with jit serialization.