Skip to content

Conversation

@Lunderberg
Copy link
Contributor

A follow-up to #16745. For Relax functions produced in TVMScript, when R.func_attrs was not present, the default was set to None instead of an empty dictionary.

A follow-up to apache#16745.  For Relax
functions produced in TVMScript, when `R.func_attrs` was not present,
the default was set to `None` instead of an empty dictionary.
@slyubomirsky
Copy link
Contributor

What is the specific reason to have this change? It might be a good invariant to have, just wondering if there was a specific motivation for it.

@Lunderberg
Copy link
Contributor Author

Overall, to remove a repeated source of bugs. Since most functions are publicly exposed, they'll have at least the "global_symbol" attribute. Frequently, utilities would be written that only worked for functions that had non-empty attributes. For example, checking if 'my_attribute' in func.attrs instead of if func.attrs is not None and 'my_attribute' in func.attrs. These instances could look reasonable for quite some time, until a function that happens to have no attributes would trigger the bug.

This class of bug was only possible because we had two entirely separate ways to represent "has no attributes". Changing the default from None to an empty DictAttrs removes the opportunity for these bugs to occur.

Copy link
Contributor

@slyubomirsky slyubomirsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I support your reasoning, this is an easily averted source of errors.

@Lunderberg Lunderberg merged commit b91d4e5 into apache:main Apr 5, 2024
@Lunderberg Lunderberg deleted the tvmscript_produce_empty_func_attrs_by_default branch April 5, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants