Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr #8920

Merged
merged 5 commits into from
Sep 8, 2021
Merged

[Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr #8920

merged 5 commits into from
Sep 8, 2021

Conversation

syang-ng
Copy link
Contributor

@syang-ng syang-ng commented Sep 3, 2021 •
edited
Loading

Fix the bug mentioned in https://discuss.tvm.apache.org/t/cannot-list-attrs-of-tvm-ir-array-via-dir/10924

Every time when we try to register a node, the size of fvisit_attrs_ will increase, and there are no other operations to unregister these nodes, so if tindex < fvisit_attrs_.size() is true, it means that the node has been registered.

template <typename T, typename TraitName>
inline ReflectionVTable::Registry ReflectionVTable::Register() {
  uint32_t tindex = T::RuntimeTypeIndex();
  if (tindex >= fvisit_attrs_.size()) {
    fvisit_attrs_.resize(tindex + 1, nullptr);
    fcreate_.resize(tindex + 1, nullptr);
    frepr_bytes_.resize(tindex + 1, nullptr);
    fsequal_reduce_.resize(tindex + 1, nullptr);
    fshash_reduce_.resize(tindex + 1, nullptr);
  }
  // functor that implements the redirection.
  fvisit_attrs_[tindex] = ::tvm::detail::SelectVisitAttrs<T, TraitName>::VisitAttrs;

  fsequal_reduce_[tindex] = ::tvm::detail::SelectSEqualReduce<T, TraitName>::SEqualReduce;

  fshash_reduce_[tindex] = ::tvm::detail::SelectSHashReduce<T, TraitName>::SHashReduce;

  return Registry(this, tindex);
}

Therefore, I just use the tindex >= fvisit_attrs_.size() to determine whether it has been registered. And if fvisit_attrs_[tindex] != nullptr exists, I will execute fvisit_attrs_[tindex](self, visitor); to get its attribute.

@junrushao1994 @comaniac

tqchen
tqchen reviewed Sep 3, 2021
View reviewed changes
include/tvm/node/reflection.h Outdated
@@ -386,11 +386,13 @@ inline ReflectionVTable::Registry ReflectionVTable::Register() {

inline void ReflectionVTable::VisitAttrs(Object* self, AttrVisitor* visitor) const {
uint32_t tindex = self->type_index();
if (tindex >= fvisit_attrs_.size() || fvisit_attrs_[tindex] == nullptr) {
if (tindex >= fvisit_attrs_.size()) {
Copy link
Member

Choose a reason for hiding this comment

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

This might cause some of the unintended behavior(since for certain object we might want VisitAttrs to exist and would like to detecting problem by seeing the error). Would be great if we can directly overload the getattr and dir on the python container object instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your valuable advice! I re-submit a new commit to fix this bug by overloading the __getattr__ and __dir__ function on the python container object. Since this commit only focuses on the front-end python container, I think it would not bring any unintended behaviors now.

@junrushao
Copy link
Member

CC: @comaniac

comaniac
comaniac approved these changes Sep 3, 2021
View reviewed changes
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM. Do we need a test for this?

@syang-ng
Copy link
Contributor Author

syang-ng commented Sep 3, 2021

LGTM. Do we need a test for this?

I agree. So I send a new commit about adding a new test for dir and getattr of python container object in tests/python/unittest/test_ir_container.py

comaniac
comaniac requested changes Sep 3, 2021
View reviewed changes
tests/python/unittest/test_ir_container.py Outdated
@@ -34,6 +34,21 @@ def test_array_save_load_json():
assert a_loaded[1].value == 2


def test_dir_array():
a = tvm.runtime.convert([1, 2, 3])
dir(a)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to check its outputs instead of just letting it go. It could be at least assert dir(a).

tests/python/unittest/test_ir_container.py Outdated
Comment on lines 46 to 49
try:
getattr(a, "test_key")
except AttributeError:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are expecting an exception, please use

with pytest.raise(AttributeError):
    getattr(a, "test_key")

In this particular case, it might be even simpler :

assert not hasattr(a, "test_key")

tests/python/unittest/test_ir_container.py Outdated
Comment on lines 128 to 131
test_dir_array()
test_dir_map()
test_getattr_array()
test_getattr_map()
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind to help change to the pytest style?

if __name__ == "__main__":
    pytest.main([__file__])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your valuable advice! I have rewritten these four test cases and changed the test file to the pytest style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you help review this new commit as it passes all checks :-D @comaniac

@junrushao
Copy link
Member

@comaniac mind taking another look? thanks!

comaniac
comaniac approved these changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@comaniac comaniac merged commit f8b1df4 into apache:main Sep 8, 2021
@comaniac
Copy link
Contributor

comaniac commented Sep 8, 2021

Thanks @syang-ng @junrushao1994 @tqchen

ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
@ylc
[Bugfix] Fix visit_attrs error if its function pointer is equal to nu…
aaf2133 
…llptr (apache#8920)

* fix visit_attrs equals nullptr on python container object

* add a test a for python container object about function dir and getattr

* change test_ir_container.py to the pytest style

* update the style to fix ci error

* update the style of ir container to fix ci error
@syang-ng syang-ng deleted the fix-visit-attrs branch November 16, 2021 08:30
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@ylc
[Bugfix] Fix visit_attrs error if its function pointer is equal to nu…
d04fd60 
…llptr (apache#8920)

* fix visit_attrs equals nullptr on python container object

* add a test a for python container object about function dir and getattr

* change test_ir_container.py to the pytest style

* update the style to fix ci error

* update the style of ir container to fix ci error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tqchen tqchen tqchen left review comments

@comaniac comaniac comaniac approved these changes

@jroesch jroesch Awaiting requested review from jroesch

@junrushao junrushao Awaiting requested review from junrushao

@vinx13 vinx13 Awaiting requested review from vinx13

@areusch areusch Awaiting requested review from areusch

@merrymercy merrymercy Awaiting requested review from merrymercy

@yzhliu yzhliu Awaiting requested review from yzhliu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@syang-ng @junrushao @comaniac @tqchen