You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/09/03 08:32:54 UTC
[GitHub] [tvm] syang-ng opened a new pull request #8920: [Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr
syang-ng opened a new pull request #8920:
URL: https://github.com/apache/tvm/pull/8920
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.
```c++
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8920: [Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8920:
URL: https://github.com/apache/tvm/pull/8920#issuecomment-915540902
@comaniac mind taking another look? thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] syang-ng commented on a change in pull request #8920: [Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr
Posted by GitBox <gi...@apache.org>.
syang-ng commented on a change in pull request #8920:
URL: https://github.com/apache/tvm/pull/8920#discussion_r703107709
##########
File path: tests/python/unittest/test_ir_container.py
##########
@@ -91,5 +125,10 @@ def test_ndarray_container():
test_map()
test_array_save_load_json()
test_map_save_load_json()
+ test_dir_array()
+ test_dir_map()
+ test_getattr_array()
+ test_getattr_map()
Review comment:
Could you help review this new commit as it passes all checks :-D @comaniac
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] syang-ng commented on pull request #8920: [Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr
Posted by GitBox <gi...@apache.org>.
syang-ng commented on pull request #8920:
URL: https://github.com/apache/tvm/pull/8920#issuecomment-912728456
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] syang-ng commented on a change in pull request #8920: [Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr
Posted by GitBox <gi...@apache.org>.
syang-ng commented on a change in pull request #8920:
URL: https://github.com/apache/tvm/pull/8920#discussion_r702222859
##########
File path: tests/python/unittest/test_ir_container.py
##########
@@ -91,5 +125,10 @@ def test_ndarray_container():
test_map()
test_array_save_load_json()
test_map_save_load_json()
+ test_dir_array()
+ test_dir_map()
+ test_getattr_array()
+ test_getattr_map()
Review comment:
Thank you for your valuable advice! I have rewritten these four test cases and changed the test file to the pytest style.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac merged pull request #8920: [Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr
Posted by GitBox <gi...@apache.org>.
comaniac merged pull request #8920:
URL: https://github.com/apache/tvm/pull/8920
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] syang-ng commented on a change in pull request #8920: [Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr
Posted by GitBox <gi...@apache.org>.
syang-ng commented on a change in pull request #8920:
URL: https://github.com/apache/tvm/pull/8920#discussion_r702035289
##########
File path: include/tvm/node/reflection.h
##########
@@ -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()) {
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on a change in pull request #8920: [Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #8920:
URL: https://github.com/apache/tvm/pull/8920#discussion_r702116775
##########
File path: tests/python/unittest/test_ir_container.py
##########
@@ -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)
+
+
+def test_getattr_array():
+ a = tvm.runtime.convert([1, 2, 3])
+ type_key = getattr(a, "type_key")
+ assert type_key == "Array"
+ try:
+ getattr(a, "test_key")
+ except AttributeError:
+ pass
Review comment:
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")
```
##########
File path: tests/python/unittest/test_ir_container.py
##########
@@ -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)
Review comment:
It would be better to check its outputs instead of just letting it go. It could be at least `assert dir(a)`.
##########
File path: tests/python/unittest/test_ir_container.py
##########
@@ -91,5 +125,10 @@ def test_ndarray_container():
test_map()
test_array_save_load_json()
test_map_save_load_json()
+ test_dir_array()
+ test_dir_map()
+ test_getattr_array()
+ test_getattr_map()
Review comment:
Would you mind to help change to the pytest style?
```python
if __name__ == "__main__":
pytest.main([__file__])
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8920: [Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8920:
URL: https://github.com/apache/tvm/pull/8920#issuecomment-912672424
CC: @comaniac
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] comaniac commented on pull request #8920: [Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr
Posted by GitBox <gi...@apache.org>.
comaniac commented on pull request #8920:
URL: https://github.com/apache/tvm/pull/8920#issuecomment-915560628
Thanks @syang-ng @junrushao1994 @tqchen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #8920: [Bugfix] Fix visit_attrs error if its function pointer is equal to nullptr
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #8920:
URL: https://github.com/apache/tvm/pull/8920#discussion_r701873764
##########
File path: include/tvm/node/reflection.h
##########
@@ -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()) {
Review comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org