-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FRONTEND] Support returning a named tuple #6042
Conversation
assert False, f"Unsupported type {type(value)}" | ||
|
||
vals = [fn(v) for v in value] | ||
types = [v.type for v in vals] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you're at it, it might be worth fixing this to work with tuples containing None (which will decay here and no longer have a type). Locally I have this as [v.type if v is not None else constexpr for v in vals]
but I'm not entirely sure this is right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we even support return None
.
I would defer it to another PR. Seems like there are more problems
|
||
def _apply_to_tuple_values(value, fn): | ||
if _is_namedtuple(type(value)): | ||
fields = value._fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand that we can have a python named tuple instead of a language.tuple
? That sounds like the real bug here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand that we can have a python named tuple instead of a language.tuple? That sounds like the real bug here.
Yeah, python named tuple is supposed to be supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't sound right to me, it should be converted to a triton tuple type by the frontend
@@ -1253,7 +1262,8 @@ def visit_Call(self, node): | |||
|
|||
if fn in self.builtin_namespace.values(): | |||
args = map(_unwrap_if_constexpr, args) | |||
return fn(*args, **kws) | |||
ret = fn(*args, **kws) | |||
return _apply_to_tuple_values(ret, lambda x: x) if _is_namedtuple(type(ret)) else ret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterbell10 Oh, I meant it's being converted here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed we convert every named tuple to a corresponding tl.tuple when created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so the _is_namedtuple
path should never actually be hit from visit_Return
only from here. Makes sense.
@@ -1253,7 +1262,8 @@ def visit_Call(self, node): | |||
|
|||
if fn in self.builtin_namespace.values(): | |||
args = map(_unwrap_if_constexpr, args) | |||
return fn(*args, **kws) | |||
ret = fn(*args, **kws) | |||
return _apply_to_tuple_values(ret, lambda x: x) if _is_namedtuple(type(ret)) else ret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so the _is_namedtuple
path should never actually be hit from visit_Return
only from here. Makes sense.
No description provided.