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 2022/07/14 21:34:29 UTC
[GitHub] [tvm] gbonik opened a new pull request, #12101: [TVMScript] Add object path tracing to StructuralEqual
gbonik opened a new pull request, #12101:
URL: https://github.com/apache/tvm/pull/12101
Motivation: when two IR objects fail a structural equality check, currently there is no easy way to find out which part of the IR caused the mismatch. In this PR, we modify the `StructuralEqual` infrastructure to also optionally return a pair of `ObjectPath` objects that point to the mismatch. (See https://github.com/apache/tvm/pull/11977). In the upcoming PRs, we will pass these paths to the TIR printer, so that it could highlight the mismatch location nicely.
Tracking issue: https://github.com/apache/tvm/issues/11912
cc @yelite @junrushao1994
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r936330391
##########
python/tvm/runtime/object_path.py:
##########
@@ -122,3 +123,14 @@ class MapValuePath(ObjectPath):
@tvm._ffi.register_object("MissingMapEntryPath")
class MissingMapEntryPath(ObjectPath):
pass
+
+
+@tvm._ffi.register_object("ObjectPathPair")
+class ObjectPathPair(Object):
+ @property
+ def lhs_path(self) -> ObjectPath:
+ return _ffi_node_api.ObjectPathPairLhsPath(self)
+
+ @property
+ def rhs_path(self) -> ObjectPath:
+ return _ffi_node_api.ObjectPathPairRhsPath(self)
Review Comment:
Ah I see. It is definitely less convenient but fortunately it's the only case :-)
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931670314
##########
include/tvm/node/structural_equal.h:
##########
@@ -198,9 +285,41 @@ class SEqualReducer : public BaseValueEqual {
/*! \return Get the internal handler. */
Handler* operator->() const { return handler_; }
+ /*! \brief Check if this reducer is tracing paths to the first mismatch. */
+ bool IsPathTracingEnabled() const { return tracing_data_ != nullptr; }
+
+ /*!
+ * \brief Get the paths of the currently compared objects.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ const ObjectPathPair& GetCurrentObjectPaths() const;
+
+ /*!
+ * \brief Specify the object paths of a detected mismatch.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ void RecordMismatchPaths(const ObjectPathPair& paths) const;
+
private:
+ bool EnumAttrsEqual(int lhs, int rhs, const void* lhs_address, const void* rhs_address) const;
+
+ bool ObjectAttrsEqual(const ObjectRef& lhs, const ObjectRef& rhs, bool map_free_vars,
+ const ObjectPathPair* paths) const;
+
+ static void GetPathsFromAttrAddressesAndStoreMismatch(const void* lhs_address,
+ const void* rhs_address,
+ const PathTracingData* tracing_data);
+
+ template <typename T>
+ static bool CompareAttributeValues(const T& lhs, const T& rhs,
+ const PathTracingData* tracing_data);
Review Comment:
There is no concrete problem in this particular case AFAICT, mainly because template instantiation is only defined and used in a single cc file.
On the other hand, in more generic usecases, we would prefer template instantiation being defined in header files so that it's discoverable by the compiler when multiple cc files refer to this method.
Therefore, it's somehow a personal preference (so it's subjective, not any general requirement) that I either define both instantiation and declaration in header file, or both in cc files
--
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] gbonik commented on a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
gbonik commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931437519
##########
include/tvm/node/structural_equal.h:
##########
@@ -56,6 +57,31 @@ class BaseValueEqual {
}
};
+/*!
+ * \brief Pair of `ObjectPath`s, one for each object being tested for structural equality.
+ */
+struct ObjectPathPair {
+ ObjectPath lhs_path;
+ ObjectPath rhs_path;
+};
+
+// Could be replaced with std::optional<ObjectPathPair>
+class OptionalObjectPathPair {
+ public:
+ OptionalObjectPathPair() = default;
+
+ OptionalObjectPathPair(const ObjectPathPair& p) // NOLINT(runtime/explicit)
+ : lhs_path(p.lhs_path), rhs_path(p.rhs_path) {}
+
+ bool defined() const { return lhs_path.defined(); }
+
+ ObjectPathPair value() const { return {lhs_path.value(), rhs_path.value()}; }
+
+ private:
+ Optional<ObjectPath> lhs_path;
+ Optional<ObjectPath> rhs_path;
+};
+
Review Comment:
That's correct, but making it a TVM object would require heap allocation.
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931571269
##########
include/tvm/node/structural_equal.h:
##########
@@ -198,9 +285,41 @@ class SEqualReducer : public BaseValueEqual {
/*! \return Get the internal handler. */
Handler* operator->() const { return handler_; }
+ /*! \brief Check if this reducer is tracing paths to the first mismatch. */
+ bool IsPathTracingEnabled() const { return tracing_data_ != nullptr; }
+
+ /*!
+ * \brief Get the paths of the currently compared objects.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ const ObjectPathPair& GetCurrentObjectPaths() const;
+
+ /*!
+ * \brief Specify the object paths of a detected mismatch.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ void RecordMismatchPaths(const ObjectPathPair& paths) const;
+
private:
+ bool EnumAttrsEqual(int lhs, int rhs, const void* lhs_address, const void* rhs_address) const;
+
+ bool ObjectAttrsEqual(const ObjectRef& lhs, const ObjectRef& rhs, bool map_free_vars,
+ const ObjectPathPair* paths) const;
+
+ static void GetPathsFromAttrAddressesAndStoreMismatch(const void* lhs_address,
+ const void* rhs_address,
+ const PathTracingData* tracing_data);
+
+ template <typename T>
+ static bool CompareAttributeValues(const T& lhs, const T& rhs,
+ const PathTracingData* tracing_data);
Review Comment:
Just curious - do you think it would make more sense to move those two methods to the cc file? The primary concern I'm having is that `CompareAttributeValues` is a templated method whose instantiation is all inside a cc file. If we care about visibility, we could introduce a friend class `SEqualReducerHelper` which has those two methods as static members
--
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] gbonik commented on a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
gbonik commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r923570472
##########
src/node/reflection.cc:
##########
@@ -281,4 +281,43 @@ TVM_REGISTER_GLOBAL("node.NodeGetAttr").set_body(NodeGetAttr);
TVM_REGISTER_GLOBAL("node.NodeListAttrNames").set_body(NodeListAttrNames);
TVM_REGISTER_GLOBAL("node.MakeNode").set_body(MakeNode);
+
+namespace {
+// Attribute visitor class for finding the attribute key by its address
+class GetAttrKeyByAddressVisitor : public AttrVisitor {
+ public:
+ explicit GetAttrKeyByAddressVisitor(const void* attr_address)
+ : attr_address_(attr_address), key_(nullptr) {}
+
+ void Visit(const char* key, double* value) final { DoVisit(key, value); }
+ void Visit(const char* key, int64_t* value) final { DoVisit(key, value); }
+ void Visit(const char* key, uint64_t* value) final { DoVisit(key, value); }
+ void Visit(const char* key, int* value) final { DoVisit(key, value); }
+ void Visit(const char* key, bool* value) final { DoVisit(key, value); }
+ void Visit(const char* key, std::string* value) final { DoVisit(key, value); }
+ void Visit(const char* key, void** value) final { DoVisit(key, value); }
+ void Visit(const char* key, DataType* value) final { DoVisit(key, value); }
+ void Visit(const char* key, runtime::NDArray* value) final { DoVisit(key, value); }
+ void Visit(const char* key, runtime::ObjectRef* value) final { DoVisit(key, value); }
+
+ const char* GetKey() const { return key_; }
+
+ private:
+ const void* attr_address_;
+ const char* key_;
+
+ void DoVisit(const char* key, const void* candidate) {
+ if (attr_address_ == candidate) {
+ key_ = key;
+ }
+ }
+};
+} // anonymous namespace
+
+const char* GetAttrKeyByAddress(const Object* object, const void* attr_address) {
+ GetAttrKeyByAddressVisitor visitor(attr_address);
+ ReflectionVTable::Global()->VisitAttrs(const_cast<Object*>(object), &visitor);
+ return visitor.GetKey();
Review Comment:
The idea is that it returns `nullptr` when the attribute does not exist. In this case, when tracing paths, we will create a special `UnknownAttributeAccessPath` that doesn't compare equal even to itself. This is to prevent the code from failing hard: if some of the tracing code is incomplete, I'd rather have the printing still work without highlighting, than crash with an exception.
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931524593
##########
include/tvm/node/structural_equal.h:
##########
@@ -99,7 +125,10 @@ class StructuralEqual : public BaseValueEqual {
* equality checking. Instead, it can store the necessary equality conditions
* and check later via an internally managed stack.
*/
-class SEqualReducer : public BaseValueEqual {
+class SEqualReducer {
Review Comment:
Given this PR removes the dependency from `SEqualReducer` to `BaseValueEqual`, now the equality checking super class is used only by the class `StructuralEqual`, so it might make more sense to completely absorb `BaseValueEqual` into `StructuralEqual`
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931528838
##########
include/tvm/node/structural_equal.h:
##########
@@ -99,7 +125,10 @@ class StructuralEqual : public BaseValueEqual {
* equality checking. Instead, it can store the necessary equality conditions
* and check later via an internally managed stack.
*/
-class SEqualReducer : public BaseValueEqual {
+class SEqualReducer {
Review Comment:
Oh looks like we still depend on `BaseValueEqual` [here](https://github.com/gbonik/tvm/blob/4c28f15da2268db5da587b87113dfc5288ef76a3/src/node/structural_equal.cc#L91-L92)...Then I dont have strong opinions. Just let me know what you think!
--
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] gbonik commented on a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
gbonik commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r932584270
##########
include/tvm/node/structural_equal.h:
##########
@@ -198,9 +285,41 @@ class SEqualReducer : public BaseValueEqual {
/*! \return Get the internal handler. */
Handler* operator->() const { return handler_; }
+ /*! \brief Check if this reducer is tracing paths to the first mismatch. */
+ bool IsPathTracingEnabled() const { return tracing_data_ != nullptr; }
+
+ /*!
+ * \brief Get the paths of the currently compared objects.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ const ObjectPathPair& GetCurrentObjectPaths() const;
+
+ /*!
+ * \brief Specify the object paths of a detected mismatch.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ void RecordMismatchPaths(const ObjectPathPair& paths) const;
+
private:
+ bool EnumAttrsEqual(int lhs, int rhs, const void* lhs_address, const void* rhs_address) const;
+
+ bool ObjectAttrsEqual(const ObjectRef& lhs, const ObjectRef& rhs, bool map_free_vars,
+ const ObjectPathPair* paths) const;
+
+ static void GetPathsFromAttrAddressesAndStoreMismatch(const void* lhs_address,
+ const void* rhs_address,
+ const PathTracingData* tracing_data);
+
+ template <typename T>
+ static bool CompareAttributeValues(const T& lhs, const T& rhs,
+ const PathTracingData* tracing_data);
Review Comment:
I think I mostly understand your point, but I'm missing one thing: why is this specific to function templates? For example, the non-template helper function `GetPathsFromAttrAddressesAndStoreMismatch` just above is also private but we have to put its declaration in the header file, because it is a static function in our class (which we need because of C++ visibility rules).
Even if we go with the `SEqualReducerHelper` approach, we still need to leak some details in the header file because we need to either declare it as a static class or as a friend class.
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931430515
##########
include/tvm/node/structural_equal.h:
##########
@@ -56,6 +57,31 @@ class BaseValueEqual {
}
};
+/*!
+ * \brief Pair of `ObjectPath`s, one for each object being tested for structural equality.
+ */
+struct ObjectPathPair {
+ ObjectPath lhs_path;
+ ObjectPath rhs_path;
+};
+
+// Could be replaced with std::optional<ObjectPathPair>
+class OptionalObjectPathPair {
+ public:
+ OptionalObjectPathPair() = default;
+
+ OptionalObjectPathPair(const ObjectPathPair& p) // NOLINT(runtime/explicit)
+ : lhs_path(p.lhs_path), rhs_path(p.rhs_path) {}
+
+ bool defined() const { return lhs_path.defined(); }
+
+ ObjectPathPair value() const { return {lhs_path.value(), rhs_path.value()}; }
+
+ private:
+ Optional<ObjectPath> lhs_path;
+ Optional<ObjectPath> rhs_path;
+};
+
Review Comment:
Quick question: if `ObjectPathPair` is a TVM object, then we don't need to define another class as the optional wrapper - is my understanding correct?
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931675308
##########
src/tir/analysis/deep_equal.cc:
##########
@@ -62,7 +67,7 @@ bool ExprDeepEqual::operator()(const PrimExpr& lhs, const PrimExpr& rhs) const {
if (lhs.as<AnyNode>()) {
return false;
}
- return DeepCmpSEqualHandler().SEqualReduce(lhs, rhs, false);
+ return DeepCmpSEqualHandler().SEqualReduce(lhs, rhs, false, {});
Review Comment:
nit: prefer more explicit instantiation of `Optional<>`
```suggestion
return DeepCmpSEqualHandler().SEqualReduce(lhs, rhs, false, NullOpt);
```
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931685347
##########
src/node/structural_equal.cc:
##########
@@ -42,6 +62,133 @@ bool ReflectionVTable::SEqualReduce(const Object* self, const Object* other,
return fsequal_reduce_[tindex](self, other, equal);
}
+struct SEqualReducer::PathTracingData {
+ ObjectPathPair current_paths;
+ ObjectRef lhs_object;
+ ObjectRef rhs_object;
+ Optional<ObjectPathPair>* first_mismatch;
+
+ ObjectPathPair GetPathsForAttrs(const ObjectRef& lhs, const ObjectRef& rhs) const {
+ Optional<String> lhs_attr_key = GetAttrKeyByAddress(lhs_object.get(), &lhs);
+ Optional<String> rhs_attr_key = GetAttrKeyByAddress(rhs_object.get(), &rhs);
+ return ObjectPathPair(current_paths->lhs_path->Attr(lhs_attr_key),
+ current_paths->rhs_path->Attr(rhs_attr_key));
+ }
+};
+
+bool SEqualReducer::operator()(const ObjectRef& lhs, const ObjectRef& rhs) const {
+ if (tracing_data_ == nullptr) {
+ // Fast path: no tracing
+ return handler_->SEqualReduce(lhs, rhs, map_free_vars_, {});
+ }
+ return ObjectAttrsEqual(lhs, rhs, map_free_vars_, nullptr);
+}
+
+bool SEqualReducer::DefEqual(const ObjectRef& lhs, const ObjectRef& rhs) {
+ if (tracing_data_ == nullptr) {
+ // Fast path: no tracing
+ return handler_->SEqualReduce(lhs, rhs, true, {});
+ }
+ return ObjectAttrsEqual(lhs, rhs, true, nullptr);
+}
+
+/* static */ void SEqualReducer::GetPathsFromAttrAddressesAndStoreMismatch(
+ const void* lhs_address, const void* rhs_address, const PathTracingData* tracing_data) {
+ if (tracing_data != nullptr && !tracing_data->first_mismatch->defined()) {
+ Optional<String> lhs_attr_key =
+ GetAttrKeyByAddress(tracing_data->lhs_object.get(), lhs_address);
+ Optional<String> rhs_attr_key =
+ GetAttrKeyByAddress(tracing_data->rhs_object.get(), rhs_address);
+ *tracing_data->first_mismatch =
+ ObjectPathPair(tracing_data->current_paths->lhs_path->Attr(lhs_attr_key),
+ tracing_data->current_paths->rhs_path->Attr(rhs_attr_key));
+ }
+}
+
+template <typename T>
+/* static */ bool SEqualReducer::CompareAttributeValues(const T& lhs, const T& rhs,
+ const PathTracingData* tracing_data) {
+ if (BaseValueEqual()(lhs, rhs)) {
+ return true;
+ } else {
+ GetPathsFromAttrAddressesAndStoreMismatch(&lhs, &rhs, tracing_data);
+ return false;
+ }
+}
+
+bool SEqualReducer::operator()(const double& lhs, const double& rhs) const {
+ return CompareAttributeValues(lhs, rhs, tracing_data_);
+}
+
+bool SEqualReducer::operator()(const int64_t& lhs, const int64_t& rhs) const {
+ return CompareAttributeValues(lhs, rhs, tracing_data_);
+}
+
+bool SEqualReducer::operator()(const uint64_t& lhs, const uint64_t& rhs) const {
+ return CompareAttributeValues(lhs, rhs, tracing_data_);
+}
+
+bool SEqualReducer::operator()(const int& lhs, const int& rhs) const {
+ return CompareAttributeValues(lhs, rhs, tracing_data_);
+}
+
+bool SEqualReducer::operator()(const bool& lhs, const bool& rhs) const {
+ return CompareAttributeValues(lhs, rhs, tracing_data_);
+}
+
+bool SEqualReducer::operator()(const std::string& lhs, const std::string& rhs) const {
+ return CompareAttributeValues(lhs, rhs, tracing_data_);
+}
+
+bool SEqualReducer::operator()(const DataType& lhs, const DataType& rhs) const {
+ return CompareAttributeValues(lhs, rhs, tracing_data_);
+}
+
+bool SEqualReducer::EnumAttrsEqual(int lhs, int rhs, const void* lhs_address,
+ const void* rhs_address) const {
+ if (lhs == rhs) {
+ return true;
+ } else {
+ GetPathsFromAttrAddressesAndStoreMismatch(lhs_address, rhs_address, tracing_data_);
+ return false;
+ }
+}
+
+const ObjectPathPair& SEqualReducer::GetCurrentObjectPaths() const {
+ ICHECK(tracing_data_ != nullptr)
+ << "GetCurrentObjectPaths() can only be called when path tracing is enabled";
+ return tracing_data_->current_paths;
+}
+
+void SEqualReducer::RecordMismatchPaths(const ObjectPathPair& paths) const {
+ ICHECK(tracing_data_ != nullptr)
+ << "RecordMismatchPaths() can only be called when path tracing is enabled";
+ if (!tracing_data_->first_mismatch->defined()) {
+ *tracing_data_->first_mismatch = paths;
+ }
+}
+
+bool SEqualReducer::ObjectAttrsEqual(const ObjectRef& lhs, const ObjectRef& rhs, bool map_free_vars,
+ const ObjectPathPair* paths) const {
+ if (tracing_data_ == nullptr) {
+ // Fast path: no tracing
+ return handler_->SEqualReduce(lhs, rhs, map_free_vars, {});
Review Comment:
nit
```suggestion
return handler_->SEqualReduce(lhs, rhs, map_free_vars, NullOpt);
```
--
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 merged pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 merged PR #12101:
URL: https://github.com/apache/tvm/pull/12101
--
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] gbonik commented on a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
gbonik commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r932591801
##########
tests/python/unittest/test_container_structural_equal.py:
##########
@@ -0,0 +1,150 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import pytest
+
+import tvm
+from tvm.ir.base import get_first_structural_mismatch
+from tvm.runtime import ObjectPath
+
+
+def get_first_mismatch_ensure_symmetry(a, b):
+ mismatch = get_first_structural_mismatch(a, b)
+ mismatch_swapped = get_first_structural_mismatch(b, a)
+
+ if mismatch is None and mismatch_swapped is None:
+ return None
+
+ if (
+ mismatch is None
+ or mismatch_swapped is None
+ or mismatch[0] != mismatch_swapped[1]
+ or mismatch[1] != mismatch_swapped[0]
+ ):
+ raise AssertionError(
+ "get_first_structural_mismatch(a, b) and get_first_structural_mismatch(b, a) returned"
+ " inconsistent results '{}' and '{}' for a='{}', b='{}'".format(
+ mismatch, mismatch_swapped, a, b
+ )
+ )
+
+ a_path, b_path = mismatch
+ b_path_swapped, a_path_swapped = mismatch_swapped
+ assert a_path == a_path_swapped
+ assert b_path == b_path_swapped
+
+ return mismatch
+
+
+@pytest.mark.parametrize(
+ "a, b, expected_a_path, expected_b_path",
+ [
+ (
+ [1, 2, 3],
+ [1, 4, 3],
+ ObjectPath.root().array_index(1).attr("value"),
+ ObjectPath.root().array_index(1).attr("value"),
+ ),
+ (
+ [1, 2, 3],
+ [10, 2, 30],
+ ObjectPath.root().array_index(0).attr("value"),
+ ObjectPath.root().array_index(0).attr("value"),
+ ),
+ (
+ [1, 3, 4],
+ [1, 2, 3, 4],
+ ObjectPath.root().array_index(1).attr("value"),
+ ObjectPath.root().array_index(1).attr("value"),
+ ),
+ (
+ [1, 2, 3],
+ [1, 2, 3, 4],
+ ObjectPath.root().missing_array_element(3),
+ ObjectPath.root().array_index(3),
+ ),
+ (
+ [],
+ [1],
+ ObjectPath.root().missing_array_element(0),
+ ObjectPath.root().array_index(0),
+ ),
+ ],
+)
+def test_array_structural_mismatch(a, b, expected_a_path, expected_b_path):
+ a = tvm.runtime.convert(a)
+ b = tvm.runtime.convert(b)
+ a_path, b_path = get_first_mismatch_ensure_symmetry(a, b)
+ assert a_path == expected_a_path
+ assert b_path == expected_b_path
+
+
+@pytest.mark.parametrize(
+ "contents",
+ [
+ [],
+ [1],
+ [1, 2, 3],
+ ],
+)
+def test_array_structural_equal_to_self(contents):
+ a = tvm.runtime.convert(list(contents))
+ b = tvm.runtime.convert(list(contents))
+ assert get_first_mismatch_ensure_symmetry(a, b) is None
+
+
+@pytest.mark.parametrize(
+ "a, b, expected_a_path, expected_b_path",
+ [
+ (
+ dict(a=3, b=4),
+ dict(a=3, b=5),
+ ObjectPath.root().map_value("b").attr("value"),
+ ObjectPath.root().map_value("b").attr("value"),
+ ),
+ (
+ dict(a=3, b=4),
+ dict(a=3, b=4, c=5),
+ ObjectPath.root().missing_map_entry(),
+ ObjectPath.root().map_value("c"),
+ ),
+ ],
+)
+def test_string_map_structural_mismatch(a, b, expected_a_path, expected_b_path):
+ a = tvm.runtime.convert(a)
+ b = tvm.runtime.convert(b)
+ a_path, b_path = get_first_mismatch_ensure_symmetry(a, b)
+ assert a_path == expected_a_path
+ assert b_path == expected_b_path
+
+
+@pytest.mark.parametrize(
+ "contents",
+ [
+ dict(),
+ dict(a=1),
+ dict(a=3, b=4, c=5),
+ ],
+)
+def test_string_structural_equal_to_self(contents):
+ a = tvm.runtime.convert(dict(contents))
+ b = tvm.runtime.convert(dict(contents))
+ assert get_first_mismatch_ensure_symmetry(a, b) is None
+
+
+# The behavior of structural equality for maps with non-string keys is fairly specific
+# to IR variables because it assumes that map keys have been "mapped" using
+# `SEqualReducer::FreeVarEqualImpl()`. So we leave this case to TIR tests.
Review Comment:
Done.
Seems unnecessary though -- why not simply run it like `pytest /path/to/file`? (And if you need the debugger then it's `gdb --args python -m pytest /path/to/file` -- seems simple enough)
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r936363849
##########
include/tvm/node/structural_equal.h:
##########
@@ -198,9 +285,41 @@ class SEqualReducer : public BaseValueEqual {
/*! \return Get the internal handler. */
Handler* operator->() const { return handler_; }
+ /*! \brief Check if this reducer is tracing paths to the first mismatch. */
+ bool IsPathTracingEnabled() const { return tracing_data_ != nullptr; }
+
+ /*!
+ * \brief Get the paths of the currently compared objects.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ const ObjectPathPair& GetCurrentObjectPaths() const;
+
+ /*!
+ * \brief Specify the object paths of a detected mismatch.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ void RecordMismatchPaths(const ObjectPathPair& paths) const;
+
private:
+ bool EnumAttrsEqual(int lhs, int rhs, const void* lhs_address, const void* rhs_address) const;
+
+ bool ObjectAttrsEqual(const ObjectRef& lhs, const ObjectRef& rhs, bool map_free_vars,
+ const ObjectPathPair* paths) const;
+
+ static void GetPathsFromAttrAddressesAndStoreMismatch(const void* lhs_address,
+ const void* rhs_address,
+ const PathTracingData* tracing_data);
+
+ template <typename T>
+ static bool CompareAttributeValues(const T& lhs, const T& rhs,
+ const PathTracingData* tracing_data);
Review Comment:
Yeah my personal (again it's just subjective) preference is that we hide anything that's not intended to be publicly used, except for non-static methods when it requires some boilerplate code (adding helper friend classes / methods). If a method is in a header file, I would prefer to document it more or less to make it easier for others to catch up
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931672392
##########
python/tvm/runtime/object_path.py:
##########
@@ -122,3 +123,14 @@ class MapValuePath(ObjectPath):
@tvm._ffi.register_object("MissingMapEntryPath")
class MissingMapEntryPath(ObjectPath):
pass
+
+
+@tvm._ffi.register_object("ObjectPathPair")
+class ObjectPathPair(Object):
+ @property
+ def lhs_path(self) -> ObjectPath:
+ return _ffi_node_api.ObjectPathPairLhsPath(self)
+
+ @property
+ def rhs_path(self) -> ObjectPath:
+ return _ffi_node_api.ObjectPathPairRhsPath(self)
Review Comment:
If you reflection is defined inside `ObjectPathPair`, then we don't need any boilerplate code for attribute access, e.g.
```C++
// inside `ObjectPathPairNode`
void VisitAttrs(tvm::AttrVisitor* v) {
v->Visit("lhs_path", &lhs_path);
v->Visit("rhs_path", &rhs_path);
}
// in a cc file
- TVM_REGISTER_OBJECT_TYPE(ObjectPathPairNode);
+ TVM_REGISTER_NODE_TYPE(ObjectPathPairNode);
```
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r922706231
##########
include/tvm/node/reflection.h:
##########
@@ -404,5 +404,11 @@ inline bool ReflectionVTable::GetReprBytes(const Object* self, std::string* repr
}
}
+/*!
+ * \brief Given an object and an address of its attribute, return the key of the attribute.
+ * \return nullptr if no attribute with the given address exists.
+ */
+const char* GetAttrKeyByAddress(const Object* object, const void* attr_address);
Review Comment:
nit: it's adding a bit of overhead, but let's use `std::string` or `tvm::runtime::String` when possible
```suggestion
String GetAttrKeyByAddress(const Object* object, const void* attr_address);
```
or
```suggestion
Optional<String> GetAttrKeyByAddress(const Object* object, const void* attr_address);
```
if the return could be nullable
--
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] gbonik commented on a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
gbonik commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931589275
##########
include/tvm/node/structural_equal.h:
##########
@@ -198,9 +285,41 @@ class SEqualReducer : public BaseValueEqual {
/*! \return Get the internal handler. */
Handler* operator->() const { return handler_; }
+ /*! \brief Check if this reducer is tracing paths to the first mismatch. */
+ bool IsPathTracingEnabled() const { return tracing_data_ != nullptr; }
+
+ /*!
+ * \brief Get the paths of the currently compared objects.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ const ObjectPathPair& GetCurrentObjectPaths() const;
+
+ /*!
+ * \brief Specify the object paths of a detected mismatch.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ void RecordMismatchPaths(const ObjectPathPair& paths) const;
+
private:
+ bool EnumAttrsEqual(int lhs, int rhs, const void* lhs_address, const void* rhs_address) const;
+
+ bool ObjectAttrsEqual(const ObjectRef& lhs, const ObjectRef& rhs, bool map_free_vars,
+ const ObjectPathPair* paths) const;
+
+ static void GetPathsFromAttrAddressesAndStoreMismatch(const void* lhs_address,
+ const void* rhs_address,
+ const PathTracingData* tracing_data);
+
+ template <typename T>
+ static bool CompareAttributeValues(const T& lhs, const T& rhs,
+ const PathTracingData* tracing_data);
Review Comment:
What is the actual concern about having a function template declared in the header?
I don't have a strong opinion, can move these to a helper friend class if you prefer it that way.
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931670314
##########
include/tvm/node/structural_equal.h:
##########
@@ -198,9 +285,41 @@ class SEqualReducer : public BaseValueEqual {
/*! \return Get the internal handler. */
Handler* operator->() const { return handler_; }
+ /*! \brief Check if this reducer is tracing paths to the first mismatch. */
+ bool IsPathTracingEnabled() const { return tracing_data_ != nullptr; }
+
+ /*!
+ * \brief Get the paths of the currently compared objects.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ const ObjectPathPair& GetCurrentObjectPaths() const;
+
+ /*!
+ * \brief Specify the object paths of a detected mismatch.
+ *
+ * Can only be called when `IsPathTracingEnabled()` is true.
+ */
+ void RecordMismatchPaths(const ObjectPathPair& paths) const;
+
private:
+ bool EnumAttrsEqual(int lhs, int rhs, const void* lhs_address, const void* rhs_address) const;
+
+ bool ObjectAttrsEqual(const ObjectRef& lhs, const ObjectRef& rhs, bool map_free_vars,
+ const ObjectPathPair* paths) const;
+
+ static void GetPathsFromAttrAddressesAndStoreMismatch(const void* lhs_address,
+ const void* rhs_address,
+ const PathTracingData* tracing_data);
+
+ template <typename T>
+ static bool CompareAttributeValues(const T& lhs, const T& rhs,
+ const PathTracingData* tracing_data);
Review Comment:
There is no concrete problem in this particular case AFAICT - because template instantiation is only defined and used in a single cc file.
On the other hand, in more generic usecases, we would prefer template instantiation being defined in header files so that it's discoverable by the compiler when multiple cc files refer to this method.
Therefore, it's somehow a personal preference (so it's subjective, not any general requirement) that I either define both instantiation and declaration in header file, or both in cc files
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931680959
##########
src/node/structural_equal.cc:
##########
@@ -42,6 +62,133 @@ bool ReflectionVTable::SEqualReduce(const Object* self, const Object* other,
return fsequal_reduce_[tindex](self, other, equal);
}
+struct SEqualReducer::PathTracingData {
+ ObjectPathPair current_paths;
+ ObjectRef lhs_object;
+ ObjectRef rhs_object;
+ Optional<ObjectPathPair>* first_mismatch;
+
+ ObjectPathPair GetPathsForAttrs(const ObjectRef& lhs, const ObjectRef& rhs) const {
+ Optional<String> lhs_attr_key = GetAttrKeyByAddress(lhs_object.get(), &lhs);
+ Optional<String> rhs_attr_key = GetAttrKeyByAddress(rhs_object.get(), &rhs);
+ return ObjectPathPair(current_paths->lhs_path->Attr(lhs_attr_key),
+ current_paths->rhs_path->Attr(rhs_attr_key));
+ }
+};
+
+bool SEqualReducer::operator()(const ObjectRef& lhs, const ObjectRef& rhs) const {
+ if (tracing_data_ == nullptr) {
+ // Fast path: no tracing
+ return handler_->SEqualReduce(lhs, rhs, map_free_vars_, {});
+ }
+ return ObjectAttrsEqual(lhs, rhs, map_free_vars_, nullptr);
+}
+
+bool SEqualReducer::DefEqual(const ObjectRef& lhs, const ObjectRef& rhs) {
+ if (tracing_data_ == nullptr) {
+ // Fast path: no tracing
+ return handler_->SEqualReduce(lhs, rhs, true, {});
Review Comment:
nit
```suggestion
return handler_->SEqualReduce(lhs, rhs, true, NullOpt);
```
--
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] gbonik commented on a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
gbonik commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r932588181
##########
python/tvm/runtime/object_path.py:
##########
@@ -122,3 +123,14 @@ class MapValuePath(ObjectPath):
@tvm._ffi.register_object("MissingMapEntryPath")
class MissingMapEntryPath(ObjectPath):
pass
+
+
+@tvm._ffi.register_object("ObjectPathPair")
+class ObjectPathPair(Object):
+ @property
+ def lhs_path(self) -> ObjectPath:
+ return _ffi_node_api.ObjectPathPairLhsPath(self)
+
+ @property
+ def rhs_path(self) -> ObjectPath:
+ return _ffi_node_api.ObjectPathPairRhsPath(self)
Review Comment:
The problem with this approach is that we can't `#include "reflection.h"` in the `structural_equal.h" because of a circular dependency (`reflection.h` already includes `structural_equal.h`). We could forward-declare the `AttrVisitor` class in the header, and put the implementation of `VisitAttrs` in the cc file, but we would risk getting the two out of sync. I think this is really a special case because this ObjectPathPair is part of the reflection mechanism, so I think it is okay to have the extra boilerplate for the getters.
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r936237572
##########
tests/python/unittest/test_container_structural_equal.py:
##########
@@ -0,0 +1,150 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import pytest
+
+import tvm
+from tvm.ir.base import get_first_structural_mismatch
+from tvm.runtime import ObjectPath
+
+
+def get_first_mismatch_ensure_symmetry(a, b):
+ mismatch = get_first_structural_mismatch(a, b)
+ mismatch_swapped = get_first_structural_mismatch(b, a)
+
+ if mismatch is None and mismatch_swapped is None:
+ return None
+
+ if (
+ mismatch is None
+ or mismatch_swapped is None
+ or mismatch[0] != mismatch_swapped[1]
+ or mismatch[1] != mismatch_swapped[0]
+ ):
+ raise AssertionError(
+ "get_first_structural_mismatch(a, b) and get_first_structural_mismatch(b, a) returned"
+ " inconsistent results '{}' and '{}' for a='{}', b='{}'".format(
+ mismatch, mismatch_swapped, a, b
+ )
+ )
+
+ a_path, b_path = mismatch
+ b_path_swapped, a_path_swapped = mismatch_swapped
+ assert a_path == a_path_swapped
+ assert b_path == b_path_swapped
+
+ return mismatch
+
+
+@pytest.mark.parametrize(
+ "a, b, expected_a_path, expected_b_path",
+ [
+ (
+ [1, 2, 3],
+ [1, 4, 3],
+ ObjectPath.root().array_index(1).attr("value"),
+ ObjectPath.root().array_index(1).attr("value"),
+ ),
+ (
+ [1, 2, 3],
+ [10, 2, 30],
+ ObjectPath.root().array_index(0).attr("value"),
+ ObjectPath.root().array_index(0).attr("value"),
+ ),
+ (
+ [1, 3, 4],
+ [1, 2, 3, 4],
+ ObjectPath.root().array_index(1).attr("value"),
+ ObjectPath.root().array_index(1).attr("value"),
+ ),
+ (
+ [1, 2, 3],
+ [1, 2, 3, 4],
+ ObjectPath.root().missing_array_element(3),
+ ObjectPath.root().array_index(3),
+ ),
+ (
+ [],
+ [1],
+ ObjectPath.root().missing_array_element(0),
+ ObjectPath.root().array_index(0),
+ ),
+ ],
+)
+def test_array_structural_mismatch(a, b, expected_a_path, expected_b_path):
+ a = tvm.runtime.convert(a)
+ b = tvm.runtime.convert(b)
+ a_path, b_path = get_first_mismatch_ensure_symmetry(a, b)
+ assert a_path == expected_a_path
+ assert b_path == expected_b_path
+
+
+@pytest.mark.parametrize(
+ "contents",
+ [
+ [],
+ [1],
+ [1, 2, 3],
+ ],
+)
+def test_array_structural_equal_to_self(contents):
+ a = tvm.runtime.convert(list(contents))
+ b = tvm.runtime.convert(list(contents))
+ assert get_first_mismatch_ensure_symmetry(a, b) is None
+
+
+@pytest.mark.parametrize(
+ "a, b, expected_a_path, expected_b_path",
+ [
+ (
+ dict(a=3, b=4),
+ dict(a=3, b=5),
+ ObjectPath.root().map_value("b").attr("value"),
+ ObjectPath.root().map_value("b").attr("value"),
+ ),
+ (
+ dict(a=3, b=4),
+ dict(a=3, b=4, c=5),
+ ObjectPath.root().missing_map_entry(),
+ ObjectPath.root().map_value("c"),
+ ),
+ ],
+)
+def test_string_map_structural_mismatch(a, b, expected_a_path, expected_b_path):
+ a = tvm.runtime.convert(a)
+ b = tvm.runtime.convert(b)
+ a_path, b_path = get_first_mismatch_ensure_symmetry(a, b)
+ assert a_path == expected_a_path
+ assert b_path == expected_b_path
+
+
+@pytest.mark.parametrize(
+ "contents",
+ [
+ dict(),
+ dict(a=1),
+ dict(a=3, b=4, c=5),
+ ],
+)
+def test_string_structural_equal_to_self(contents):
+ a = tvm.runtime.convert(dict(contents))
+ b = tvm.runtime.convert(dict(contents))
+ assert get_first_mismatch_ensure_symmetry(a, b) is None
+
+
+# The behavior of structural equality for maps with non-string keys is fairly specific
+# to IR variables because it assumes that map keys have been "mapped" using
+# `SEqualReducer::FreeVarEqualImpl()`. So we leave this case to TIR tests.
Review Comment:
Yeah I personally don't think it necessary either...Just seeing other test files adding those lines
--
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] gbonik commented on a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
gbonik commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r932588181
##########
python/tvm/runtime/object_path.py:
##########
@@ -122,3 +123,14 @@ class MapValuePath(ObjectPath):
@tvm._ffi.register_object("MissingMapEntryPath")
class MissingMapEntryPath(ObjectPath):
pass
+
+
+@tvm._ffi.register_object("ObjectPathPair")
+class ObjectPathPair(Object):
+ @property
+ def lhs_path(self) -> ObjectPath:
+ return _ffi_node_api.ObjectPathPairLhsPath(self)
+
+ @property
+ def rhs_path(self) -> ObjectPath:
+ return _ffi_node_api.ObjectPathPairRhsPath(self)
Review Comment:
The problem with this approach is that we can't `#include "reflection.h"` in the `structural_equal.h` because of a circular dependency (`reflection.h` already includes `structural_equal.h`). We could forward-declare the `AttrVisitor` class in the header, and put the implementation of `VisitAttrs` in the cc file, but we would risk getting the two out of sync. I think this is really a special case because this ObjectPathPair is part of the reflection mechanism, so I think it is okay to have the extra boilerplate for the getters.
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931675975
##########
tests/python/unittest/test_container_structural_equal.py:
##########
@@ -0,0 +1,150 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import pytest
+
+import tvm
+from tvm.ir.base import get_first_structural_mismatch
+from tvm.runtime import ObjectPath
+
+
+def get_first_mismatch_ensure_symmetry(a, b):
+ mismatch = get_first_structural_mismatch(a, b)
+ mismatch_swapped = get_first_structural_mismatch(b, a)
+
+ if mismatch is None and mismatch_swapped is None:
+ return None
+
+ if (
+ mismatch is None
+ or mismatch_swapped is None
+ or mismatch[0] != mismatch_swapped[1]
+ or mismatch[1] != mismatch_swapped[0]
+ ):
+ raise AssertionError(
+ "get_first_structural_mismatch(a, b) and get_first_structural_mismatch(b, a) returned"
+ " inconsistent results '{}' and '{}' for a='{}', b='{}'".format(
+ mismatch, mismatch_swapped, a, b
+ )
+ )
+
+ a_path, b_path = mismatch
+ b_path_swapped, a_path_swapped = mismatch_swapped
+ assert a_path == a_path_swapped
+ assert b_path == b_path_swapped
+
+ return mismatch
+
+
+@pytest.mark.parametrize(
+ "a, b, expected_a_path, expected_b_path",
+ [
+ (
+ [1, 2, 3],
+ [1, 4, 3],
+ ObjectPath.root().array_index(1).attr("value"),
+ ObjectPath.root().array_index(1).attr("value"),
+ ),
+ (
+ [1, 2, 3],
+ [10, 2, 30],
+ ObjectPath.root().array_index(0).attr("value"),
+ ObjectPath.root().array_index(0).attr("value"),
+ ),
+ (
+ [1, 3, 4],
+ [1, 2, 3, 4],
+ ObjectPath.root().array_index(1).attr("value"),
+ ObjectPath.root().array_index(1).attr("value"),
+ ),
+ (
+ [1, 2, 3],
+ [1, 2, 3, 4],
+ ObjectPath.root().missing_array_element(3),
+ ObjectPath.root().array_index(3),
+ ),
+ (
+ [],
+ [1],
+ ObjectPath.root().missing_array_element(0),
+ ObjectPath.root().array_index(0),
+ ),
+ ],
+)
+def test_array_structural_mismatch(a, b, expected_a_path, expected_b_path):
+ a = tvm.runtime.convert(a)
+ b = tvm.runtime.convert(b)
+ a_path, b_path = get_first_mismatch_ensure_symmetry(a, b)
+ assert a_path == expected_a_path
+ assert b_path == expected_b_path
+
+
+@pytest.mark.parametrize(
+ "contents",
+ [
+ [],
+ [1],
+ [1, 2, 3],
+ ],
+)
+def test_array_structural_equal_to_self(contents):
+ a = tvm.runtime.convert(list(contents))
+ b = tvm.runtime.convert(list(contents))
+ assert get_first_mismatch_ensure_symmetry(a, b) is None
+
+
+@pytest.mark.parametrize(
+ "a, b, expected_a_path, expected_b_path",
+ [
+ (
+ dict(a=3, b=4),
+ dict(a=3, b=5),
+ ObjectPath.root().map_value("b").attr("value"),
+ ObjectPath.root().map_value("b").attr("value"),
+ ),
+ (
+ dict(a=3, b=4),
+ dict(a=3, b=4, c=5),
+ ObjectPath.root().missing_map_entry(),
+ ObjectPath.root().map_value("c"),
+ ),
+ ],
+)
+def test_string_map_structural_mismatch(a, b, expected_a_path, expected_b_path):
+ a = tvm.runtime.convert(a)
+ b = tvm.runtime.convert(b)
+ a_path, b_path = get_first_mismatch_ensure_symmetry(a, b)
+ assert a_path == expected_a_path
+ assert b_path == expected_b_path
+
+
+@pytest.mark.parametrize(
+ "contents",
+ [
+ dict(),
+ dict(a=1),
+ dict(a=3, b=4, c=5),
+ ],
+)
+def test_string_structural_equal_to_self(contents):
+ a = tvm.runtime.convert(dict(contents))
+ b = tvm.runtime.convert(dict(contents))
+ assert get_first_mismatch_ensure_symmetry(a, b) is None
+
+
+# The behavior of structural equality for maps with non-string keys is fairly specific
+# to IR variables because it assumes that map keys have been "mapped" using
+# `SEqualReducer::FreeVarEqualImpl()`. So we leave this case to TIR tests.
Review Comment:
nit: Let's append some epilogue to this file so that this single file could be executed independently using `python /path/to/file`.
```python
if __name__ == "__main__":
tvm.testing.main()
```
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931678426
##########
src/node/structural_hash.cc:
##########
@@ -395,12 +396,73 @@ struct ArrayNodeTrait {
}
static bool SEqualReduce(const ArrayNode* lhs, const ArrayNode* rhs, SEqualReducer equal) {
+ if (equal.IsPathTracingEnabled()) {
Review Comment:
probably off the topic, but ideally we should find a better place for defining those traits >,<
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r931680834
##########
src/node/structural_equal.cc:
##########
@@ -42,6 +62,133 @@ bool ReflectionVTable::SEqualReduce(const Object* self, const Object* other,
return fsequal_reduce_[tindex](self, other, equal);
}
+struct SEqualReducer::PathTracingData {
+ ObjectPathPair current_paths;
+ ObjectRef lhs_object;
+ ObjectRef rhs_object;
+ Optional<ObjectPathPair>* first_mismatch;
+
+ ObjectPathPair GetPathsForAttrs(const ObjectRef& lhs, const ObjectRef& rhs) const {
+ Optional<String> lhs_attr_key = GetAttrKeyByAddress(lhs_object.get(), &lhs);
+ Optional<String> rhs_attr_key = GetAttrKeyByAddress(rhs_object.get(), &rhs);
+ return ObjectPathPair(current_paths->lhs_path->Attr(lhs_attr_key),
+ current_paths->rhs_path->Attr(rhs_attr_key));
+ }
+};
+
+bool SEqualReducer::operator()(const ObjectRef& lhs, const ObjectRef& rhs) const {
+ if (tracing_data_ == nullptr) {
+ // Fast path: no tracing
+ return handler_->SEqualReduce(lhs, rhs, map_free_vars_, {});
Review Comment:
nit:
```suggestion
return handler_->SEqualReduce(lhs, rhs, map_free_vars_, NullOpt);
```
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r922706434
##########
src/node/reflection.cc:
##########
@@ -281,4 +281,43 @@ TVM_REGISTER_GLOBAL("node.NodeGetAttr").set_body(NodeGetAttr);
TVM_REGISTER_GLOBAL("node.NodeListAttrNames").set_body(NodeListAttrNames);
TVM_REGISTER_GLOBAL("node.MakeNode").set_body(MakeNode);
+
+namespace {
+// Attribute visitor class for finding the attribute key by its address
+class GetAttrKeyByAddressVisitor : public AttrVisitor {
+ public:
+ explicit GetAttrKeyByAddressVisitor(const void* attr_address)
+ : attr_address_(attr_address), key_(nullptr) {}
+
+ void Visit(const char* key, double* value) final { DoVisit(key, value); }
+ void Visit(const char* key, int64_t* value) final { DoVisit(key, value); }
+ void Visit(const char* key, uint64_t* value) final { DoVisit(key, value); }
+ void Visit(const char* key, int* value) final { DoVisit(key, value); }
+ void Visit(const char* key, bool* value) final { DoVisit(key, value); }
+ void Visit(const char* key, std::string* value) final { DoVisit(key, value); }
+ void Visit(const char* key, void** value) final { DoVisit(key, value); }
+ void Visit(const char* key, DataType* value) final { DoVisit(key, value); }
+ void Visit(const char* key, runtime::NDArray* value) final { DoVisit(key, value); }
+ void Visit(const char* key, runtime::ObjectRef* value) final { DoVisit(key, value); }
+
+ const char* GetKey() const { return key_; }
+
+ private:
+ const void* attr_address_;
+ const char* key_;
+
+ void DoVisit(const char* key, const void* candidate) {
+ if (attr_address_ == candidate) {
+ key_ = key;
+ }
+ }
+};
+} // anonymous namespace
+
+const char* GetAttrKeyByAddress(const Object* object, const void* attr_address) {
+ GetAttrKeyByAddressVisitor visitor(attr_address);
+ ReflectionVTable::Global()->VisitAttrs(const_cast<Object*>(object), &visitor);
+ return visitor.GetKey();
Review Comment:
quick question: what is the expected behavior if `GetKey()` returns nullptr?
--
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 a diff in pull request #12101: [TVMScript] Add object path tracing to StructuralEqual
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on code in PR #12101:
URL: https://github.com/apache/tvm/pull/12101#discussion_r922706231
##########
include/tvm/node/reflection.h:
##########
@@ -404,5 +404,11 @@ inline bool ReflectionVTable::GetReprBytes(const Object* self, std::string* repr
}
}
+/*!
+ * \brief Given an object and an address of its attribute, return the key of the attribute.
+ * \return nullptr if no attribute with the given address exists.
+ */
+const char* GetAttrKeyByAddress(const Object* object, const void* attr_address);
Review Comment:
nit: it's adding a bit of overhead, but let's use `std::string` or `tvm::runtime::String` when possible
```suggestion
String GetAttrKeyByAddress(const Object* object, const void* attr_address);
```
--
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