ban .sizes() and .strides() calls in derivatives.yaml#86611
ban .sizes() and .strides() calls in derivatives.yaml#86611bdhirsh wants to merge 19 commits intogh/bdhirsh/325/basefrom
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/86611
Note: Links to docs will display an error until the docs builds have been completed. ✅ No Failures, 9 PendingAs of commit ed8fae2: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
albanD
left a comment
There was a problem hiding this comment.
Should we remove the replace rule for these?
Also you should do the same for size(int) and stride(int) as well (see the regex in the replace list).
|
Agreed on both accounts - I'll update soon (deleting the existing sizes/strides regex, and erroring on |
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
|
Sadly this won't be ok to merge yet, since we need to wait on symintify'ing I'm hoping to land the big derivatives.yaml change asap though (below this in the stack). |
albanD
left a comment
There was a problem hiding this comment.
SGTM
Small optional improvement you could do.
tools/autograd/load_derivatives.py
Outdated
| ".sizes() is not supported in derivative formulas. Instead, please use the SymInt version," | ||
| + f".sym_sizes(), which returned a c10::SymIntArrayRef. formula={formula}" | ||
| ) | ||
| if re.search("\.size\([-]?\d+\)", formula): |
There was a problem hiding this comment.
we most likely want to handle ->size( here.
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
ghstack-source-id: 2553cf8 Pull Request resolved: pytorch/pytorch#86611
[ghstack-poisoned]
Stack from ghstack (oldest at bottom):