Metaprogramming facilities to support hacky_wrapper to reorder function arguments#48910
Metaprogramming facilities to support hacky_wrapper to reorder function arguments#48910smessmer wants to merge 12 commits intogh/smessmer/270/basefrom
Conversation
…on arguments This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 4b7ddf1 (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 or post in the (internal) Dr. CI Users group. This comment has been revised 49 times. |
…rder function arguments" This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
…rder function arguments" This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
bhosmer
left a comment
There was a problem hiding this comment.
Hey, not done but thought I'd flush the comment queue before I context switched away from this - back with more tmrw 😁
…rder function arguments" This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
…rder function arguments" This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
|
My high level impression on this PR is that, if we had implemented hacky wrapper as codegen, we wouldn't need any of these metaprogramming utilities, we wouldn't need to backdoor mutable argument locations as an integer sequence into the template in question, and it would be simpler to understand too. Now, I understand hacky wrapper is supposed to be temporary, so I don't want to litigate this too much. So most of my concerns boil down to (1) short term necessity of reviewing these PRs (@bhosmer, maybe you are OK with fielding the reviews here?) and (2) long term maintenance of this metaprogramming facilities (@smessmer has expressed openness to deleting them when hacky wrapper dies, or investigating adding a proper metaprogramming library like Hana as a dep to PyTorch.) |
Yup I'll be reviewing this part of the stack. Template complexity is definitely a concern, @smessmer I'll sync up offline to chat about degrees of freedom here |
…rder function arguments" This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
bhosmer
left a comment
There was a problem hiding this comment.
Given that the lifecycle for this is the same as hacky_wrapper, and we want to go to something like Hana for longer-lived metaprogramming support, I'd like to try to prune away anything here that's not strictly needed to implement #48911.
What that amounts to depends on whether arbitrary out argument patterns need to be supported - if so, I think we can significantly simplify this, and if not, we might be able to reduce it to a couple of helpers.
In either case, IMO the lifting of ints to types is overkill - the functionality we're reusing from typelist isn't significant enough to warrant the indirection; we should just implement the simple list primitives we need over integer sequences directly.
So next step is resolving the supported-patterns question in #48911, then back to here for a revisit.
…rder function arguments" This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
…rder function arguments" This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
…rder function arguments" This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
…rder function arguments" This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
…rder function arguments" This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
…rder function arguments" This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that. A PR on top of this one will implement the actual hacky_wrapper reordering. Differential Revision: [D25363337](https://our.internmc.facebook.com/intern/diff/D25363337/) [ghstack-poisoned]
|
Hey, following up on the accept: after looking at #48911, I'd suggest seriously considering making the change I proposed there, which would make this PR unnecessary. |
|
This PR got so small now that I could merge it into #48911 |
Stack from ghstack:
This doesn't implement any reordering of arguments in hacky_wrapper yet, but it implements the metaprogramming facilities needed for that.
A PR on top of this one will implement the actual hacky_wrapper reordering.
Differential Revision: D25363337