[jit] Fix handling of IntList[k] parameters#6965
[jit] Fix handling of IntList[k] parameters#6965ezyang merged 5 commits intopytorch:masterfrom elanmart:jit-intlist
Conversation
apaszke
left a comment
There was a problem hiding this comment.
Thanks a lot @elanmart and @t-vi! I think we can do this without going through the powerset, so it would be nice to change it.
Actually I think it would be even better if we had a different auto-generated JIT pass, that normalizes out all of those cases that are normally handled by the Python arg parser. Right now it's hard to handle those nodes in all optimization passes, because they can appear in a lot of possible configurations. What we should do instead is do this normalization before even attempting to run anything else on the graph. In this case, it would check if a particular node has IntList[x] args declared, and would change any int attributes under this name to correctly expanded IntList attrs. However that's a slightly bigger change, so I'm ok with merging this to unblock the sum PR, and I'll fix it later myself.
test/test_jit.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
tools/jit/gen_jit_dispatch.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
tools/jit/gen_jit_dispatch.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
tools/jit/gen_jit_dispatch.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
tools/jit/gen_jit_dispatch.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
So I must admit that the faults in the implementation strategy are probably mine here. So this is what I had in mind when I looked into making the JIT actually use python_arg_parser code for determining the function to use. |
I agree this PR doesn't seem like the nicest possible change. I'd be happy to give a try at a more elaborate one, but I'm afraid wouldn't know where to start with this. So perhaps we can unblock #6152 , and I'll try to work with @t-vi to come up with a proper solution? |
|
@elanmart can you only please tell me how many lines are there in the generated |
$ wc -l torch/csrc/jit/generated/aten_dispatch.cpp
21619 torch/csrc/jit/generated/aten_dispatch.cpp |
|
Yeah, so the powerset makes it 2x larger 😕 Eh, it's not great, but I guess we'll need to stick with it for now. |
apaszke
left a comment
There was a problem hiding this comment.
Let's clean up indices_to_replace and merge this.
tools/jit/gen_jit_dispatch.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
tools/jit/gen_jit_dispatch.py
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
* emit additional declarations and handle positional arg. case * apply minor tweaks * py-2 fix * Address Tom's comments * move logic to gen_jit_dispatch, start adding tests * add test
* squash commits * emit additional declarations and handle positional arg. case * apply minor tweaks * py-2 fix * Address Tom's comments * move logic to gen_jit_dispatch, start adding tests * add test * address review comments * address review comment * fix build issue. change argument indices to argument names. Get rid of deepcopy * py-2 flake8 fix
* squash commits * emit additional declarations and handle positional arg. case * apply minor tweaks * py-2 fix * Address Tom's comments * move logic to gen_jit_dispatch, start adding tests * add test * address review comments * address review comment * fix build issue. change argument indices to argument names. Get rid of deepcopy * py-2 flake8 fix
Fixes #6734
If there's anything wrong with this PR, it's my fault. If there's anything good about it, credit goes to @t-vi.