Add optional string support to native_functions schema #43010
Conversation
[ghstack-poisoned]
[ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit cb144dd (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 32 times. |
facebook-github-bot
added
the
oncall: jit
ghstack-source-id: 261c304 Pull Request resolved: pytorch#43010
mruberry
requested review from
bhosmer
and removed request for
apaszke and
mruberry
gchanan
added
the
triaged
bhosmer
left a comment
bhosmer
left a comment
There was a problem hiding this comment.
Looks good! Need to teach codegen in one more place (noted inline) and then I think you're good to go.
aten/src/ATen/native_parse.py
Outdated
| elif t == 'str': | ||
| t = 'std::string' | ||
| elif t == 'str?': | ||
| t = 'std::string?' |
There was a problem hiding this comment.
I think you'll need to add the inverse conversion here.
There was a problem hiding this comment.
Thanks, added.
[ghstack-poisoned]
[ghstack-poisoned]
ghstack-source-id: b933709 Pull Request resolved: pytorch#43010
[ghstack-poisoned]
ghstack-source-id: 1ef3ace Pull Request resolved: pytorch#43010
[ghstack-poisoned]
| 'int64_t': '{}.toInt()', | ||
| 'int64_t?': '{}.toOptional<int64_t>()', | ||
| 'std::string': '{}.toStringRef()', | ||
| 'std::string?': '{}.toOptional<std::string>()', |
There was a problem hiding this comment.
Following the mapping for std::string, I think it should be'std::string?': '{}.toOptionalStringRef()'
There was a problem hiding this comment.
After some experimenting, I think that would require the C++ signature of native functions be changed from c10::optional<std::string> to c10::optional<std::reference_wrapper<const std::string>> which I think is really ugly.
A nicer way to avoid the copy would be to change from std::string to c10::string_view everywhere, in which case we don't need the reference_wrapper.
There was a problem hiding this comment.
But I figure the copy is okay since std::string functions will take a copy currently anyway. So I think a string_view conversion could be left for a separate PR.
| TORCH_API void addInputs( | ||
| Node* n, | ||
| const char* name, | ||
| const c10::optional<std::string>& value); |
There was a problem hiding this comment.
const c10::optional<std::reference_wrapper<const std::string>> value
| inline c10::optional<at::MemoryFormat> memoryformatOptional(int i); | ||
| inline at::QScheme toQScheme(int i); | ||
| inline std::string string(int i); | ||
| inline c10::optional<std::string> stringOptional(int i); |
There was a problem hiding this comment.
inline c10::optional<std::reference_wrapper<const std::string>> stringOptional(int i);
| return THPUtils_unpackString(args[i]); | ||
| } | ||
|
|
||
| inline c10::optional<std::string> PythonArgs::stringOptional(int i) { |
There was a problem hiding this comment.
inline c10::optional<std::reference_wrapper<const std::string>> PythonArgs::stringOptional(int i) {
There was a problem hiding this comment.
That would cause a dangling reference.
[ghstack-poisoned]
Codecov Report
@@ Coverage Diff @@
## gh/peterbell10/2/base #43010 +/- ##
=========================================================
- Coverage 67.94% 67.94% -0.01%
=========================================================
Files 384 384
Lines 49743 49743
=========================================================
- Hits 33799 33798 -1
- Misses 15944 15945 +1
Continue to review full report at Codecov.
|
ghstack-source-id: c532432 Pull Request resolved: pytorch#43010
[ghstack-poisoned]
ghstack-source-id: 95e5fff Pull Request resolved: pytorch#43010
Differential Revision: [D23751851](https://our.internmc.facebook.com/intern/diff/D23751851) [ghstack-poisoned]
ghstack-source-id: b9ef968 Pull Request resolved: pytorch#43010
Stack from ghstack:
Differential Revision: D23751851