Re: [PR] [EXTRA] Introduce StructuralKey [tvm-ffi]
tqchen merged PR #453: URL: https://github.com/apache/tvm-ffi/pull/453 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [EXTRA] Introduce StructuralKey [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #453:
URL: https://github.com/apache/tvm-ffi/pull/453#discussion_r2810496986
##
src/ffi/extra/reflection_extra.cc:
##
@@ -133,11 +134,46 @@ inline void AccessPathRegisterReflection() {
[](const AccessPath& self, const AccessPath& other) { return
self->PathEqual(other); });
}
+int64_t StructuralKeyHash(const Any& key) {
+ const auto* key_obj = key.as();
+ TVM_FFI_ICHECK_NOTNULL(key_obj);
+ return key_obj->hash_i64;
+}
+
+bool StructuralKeyEqual(const Any& lhs, const Any& rhs) {
+ if (lhs.same_as(rhs)) {
+return true;
+ }
+ const auto* lhs_obj = lhs.as();
+ const auto* rhs_obj = rhs.as();
+ TVM_FFI_ICHECK_NOTNULL(lhs_obj);
+ TVM_FFI_ICHECK_NOTNULL(rhs_obj);
+ if (lhs_obj->hash_i64 != rhs_obj->hash_i64) {
+return false;
+ }
+ return StructuralEqual::Equal(lhs_obj->key, rhs_obj->key);
+}
+
+inline void StructuralKeyRegisterReflection() {
+ namespace refl = tvm::ffi::reflection;
+ refl::ObjectDef()
+ .def_ro("key", &StructuralKeyObj::key)
+ .def_ro("hash_i64", &StructuralKeyObj::hash_i64);
+ refl::TypeAttrDef()
+ .attr("__any_hash__", reinterpret_cast(&StructuralKeyHash))
+ .attr("__any_equal__", reinterpret_cast(&StructuralKeyEqual));
+}
+
TVM_FFI_STATIC_INIT_BLOCK() {
namespace refl = tvm::ffi::reflection;
AccessStepRegisterReflection();
AccessPathRegisterReflection();
- refl::GlobalDef().def_packed("ffi.MakeObjectFromPackedArgs",
MakeObjectFromPackedArgs);
+ StructuralKeyRegisterReflection();
+
+ refl::GlobalDef()
+ .def_packed("ffi.MakeObjectFromPackedArgs", MakeObjectFromPackedArgs)
+ .def("ffi.StructuralKey", [](Any key) { return
StructuralKey(std::move(key)); })
+ .def("ffi.StructuralKeyEqual", StructuralKeyEqual);
Review Comment:
The FFI function `StructuralKeyHash` is declared in
`python/tvm_ffi/_ffi_api.py` but it's not registered as a global function here.
This will cause a runtime error if `tvm_ffi._ffi_api.StructuralKeyHash` is
called from Python. Please register it to match the Python API definition.
```c
.def("ffi.StructuralKeyEqual", StructuralKeyEqual)
.def("ffi.StructuralKeyHash", StructuralKeyHash);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] [EXTRA] Introduce StructuralKey [tvm-ffi]
tqchen commented on PR #453: URL: https://github.com/apache/tvm-ffi/pull/453#issuecomment-3906235487 /gemini review -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [EXTRA] Introduce StructuralKey [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #453:
URL: https://github.com/apache/tvm-ffi/pull/453#discussion_r2810484670
##
include/tvm/ffi/extra/structural_key.h:
##
@@ -0,0 +1,102 @@
+/*
+ * 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.
+ */
+/*!
+ * \file tvm/ffi/extra/structural_key.h
+ * \brief Structural-key wrapper that caches structural hash.
+ */
+#ifndef TVM_FFI_EXTRA_STRUCTURAL_KEY_H_
+#define TVM_FFI_EXTRA_STRUCTURAL_KEY_H_
+
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/*!
+ * \brief Object node that contains a key and its cached structural hash.
+ */
+class StructuralKeyObj : public Object {
+ public:
+ /*! \brief The key value. */
+ Any key;
+ /*! \brief Cached structural hash of \p key, encoded as int64_t. */
+ int64_t hash_i64;
+
+ // Default constructor to support reflection-based initialization.
+ StructuralKeyObj() = default;
+ /*!
+ * \brief Construct a StructuralKeyObj from a key and cache its structural
hash.
+ * \param key The key value.
+ */
+ explicit StructuralKeyObj(Any key)
+ : key(std::move(key)),
hash_i64(static_cast(StructuralHash::Hash(this->key))) {}
+
+ /// \cond Doxygen_Suppress
+ TVM_FFI_DECLARE_OBJECT_INFO_FINAL("ffi.StructuralKey", StructuralKeyObj,
Object);
+ /// \endcond
+};
+
+/*!
+ * \brief ObjectRef wrapper for StructuralKeyObj.
+ *
+ * StructuralKey can be used to ensure that we use structural equality and
hash for the wrapped key.
+ */
+class StructuralKey : public ObjectRef {
+ public:
+ /*!
+ * \brief Construct a StructuralKey from a key.
+ * \param key The key value.
+ */
+ explicit StructuralKey(Any key) :
ObjectRef(make_object(std::move(key))) {}
+
+ bool operator==(const StructuralKey& other) const {
+if (this->same_as(other)) {
+ return true;
+}
+if (this->get()->hash_i64 != other->hash_i64) {
+ return false;
+}
+return StructuralEqual::Equal(this->get()->key, other->key);
+ }
Review Comment:
Since `StructuralKey` is an `ObjectRef`, it inherits `operator!=` from the
base class, which performs pointer inequality. By overriding `operator==` to
use structural equality without also overriding `operator!=`, you create an
inconsistency where `k1 == k2` might be true (structurally) but `k1 != k2`
could also be true (different pointers). You should explicitly define
`operator!=` to maintain consistency.
```suggestion
bool operator==(const StructuralKey& other) const {
if (this->same_as(other)) {
return true;
}
if (this->get()->hash_i64 != other->hash_i64) {
return false;
}
return StructuralEqual::Equal(this->get()->key, other->key);
}
bool operator!=(const StructuralKey& other) const {
return !(*this == other);
}
```
##
include/tvm/ffi/extra/structural_key.h:
##
@@ -0,0 +1,102 @@
+/*
+ * 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.
+ */
+/*!
+ * \file tvm/ffi/extra/structural_key.h
+ * \brief Structural-key wrapper that caches structural hash.
+ */
+#ifndef TVM_FFI_EXTRA_STRUCTURAL_KEY_H_
+#define TVM_FFI_EXTRA_STRUCTURAL_KEY_H_
+
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/*!
+ * \brief Object node that contains a key and its cac
Re: [PR] [EXTRA] Introduce StructuralKey [tvm-ffi]
tqchen commented on PR #453: URL: https://github.com/apache/tvm-ffi/pull/453#issuecomment-3906208978 /gemini review -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [EXTRA] Introduce StructuralKey [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #453:
URL: https://github.com/apache/tvm-ffi/pull/453#discussion_r2810474060
##
src/ffi/extra/reflection_extra.cc:
##
@@ -133,11 +134,42 @@ inline void AccessPathRegisterReflection() {
[](const AccessPath& self, const AccessPath& other) { return
self->PathEqual(other); });
}
+int64_t StructuralKeyHash(const Any& key) { return
key.cast()->hash_i64; }
Review Comment:
For better performance and consistency with `StructuralKeyEqual`, consider
using `as()` instead of `cast()`. `cast`
creates a temporary `ObjectRef` which involves reference counting overhead,
while `as` provides a direct pointer without this overhead. Given that this
function is an `__any_hash__` hook, the `key` is guaranteed to be of the
correct type, so a direct `as` is safe.
```suggestion
int64_t StructuralKeyHash(const Any& key) { return
key.as()->hash_i64; }
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] [EXTRA] Introduce StructuralKey [tvm-ffi]
tqchen commented on PR #453: URL: https://github.com/apache/tvm-ffi/pull/453#issuecomment-3906202507 /gemini review -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [EXTRA] Introduce StructuralKey [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #453:
URL: https://github.com/apache/tvm-ffi/pull/453#discussion_r2810467921
##
src/ffi/extra/reflection_extra.cc:
##
@@ -133,11 +134,40 @@ inline void AccessPathRegisterReflection() {
[](const AccessPath& self, const AccessPath& other) { return
self->PathEqual(other); });
}
+int64_t StructuralKeyHash(const Any& key) { return
key.cast()->hash_i64; }
+
+bool StructuralKeyEqual(const Any& lhs, const Any& rhs) {
+ auto lhs_key = lhs.try_cast();
+ auto rhs_key = rhs.try_cast();
+ if (!lhs_key.has_value() || !rhs_key.has_value()) {
+return false;
+ }
+ if ((*lhs_key)->hash_i64 != (*rhs_key)->hash_i64) {
+return false;
+ }
+ return StructuralEqual::Equal((*lhs_key)->key, (*rhs_key)->key);
+}
Review Comment:
For improved efficiency and correctness, this function can be optimized.
Since it's registered as the `__any_equal__` handler for `StructuralKey`, we
can assume that `lhs` and `rhs` are of type `StructuralKey`. Using
`as()` is more direct and avoids the overhead of
`try_cast()`, which creates temporary `ObjectRef`s.
Additionally, including a `same_as` check at the beginning is a beneficial
optimization for when the two objects are the same instance.
```suggestion
bool StructuralKeyEqual(const Any& lhs, const Any& rhs) {
if (lhs.same_as(rhs)) {
return true;
}
const auto* lhs_obj = lhs.as();
const auto* rhs_obj = rhs.as();
if (lhs_obj->hash_i64 != rhs_obj->hash_i64) {
return false;
}
return StructuralEqual::Equal(lhs_obj->key, rhs_obj->key);
}
```
##
python/tvm_ffi/structural.py:
##
@@ -0,0 +1,230 @@
+# 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.
+# pylint: disable=invalid-name
+"""Structural helper objects and functions."""
+
+from __future__ import annotations
+
+from typing import TYPE_CHECKING, Any
+
+if TYPE_CHECKING:
+from .access_path import AccessPath
+
+from . import _ffi_api
+from .core import Object
+from .registry import register_object
+
+__all__ = [
+"StructuralKey",
+"get_first_structural_mismatch",
+"structural_equal",
+"structural_hash",
+]
+
+
+def structural_equal(
+lhs: Any, rhs: Any, map_free_vars: bool = False, skip_tensor_content: bool
= False
+) -> bool:
+"""Check structural equality between two values.
+
+Structural equality compares the *shape/content structure* of two values
+instead of Python object identity. For container-like values, this means
+recursive comparison of elements/fields. For object types that provide
+structural equal hooks, those hooks are used.
+
+Parameters
+--
+lhs
+Left-hand side value.
+rhs
+Right-hand side value.
+
+map_free_vars
+Whether free variables (variables without a definition site) can be
+mapped to each other during comparison.
+
+skip_tensor_content
+Whether to skip tensor data content when comparing tensors.
+
+Returns
+---
+result
+Whether the two values are structurally equal.
+
+Examples
+
+.. code-block:: python
+
+import tvm_ffi
+
+assert tvm_ffi.structural_equal([1, 2, 3], [1, 2, 3])
+assert not tvm_ffi.structural_equal([1, 2, 3], [1, 2, 4])
+
+See Also
+
+:py:func:`tvm_ffi.structural_hash`
+Hash function compatible with structural equality.
+:py:func:`tvm_ffi.get_first_structural_mismatch`
+Mismatch diagnostics with access paths.
+
+"""
+return _ffi_api.StructuralEqual(lhs, rhs, map_free_vars,
skip_tensor_content)
+
+
+def structural_hash(
+value: Any, map_free_vars: bool = False, skip_tensor_content: bool = False
+) -> int:
+"""Compute structural hash of a value.
+
+This hash is designed to be consistent with :py:func:`structural_equal`
+under the same options. If two values are structurally equal, they should
+have the same structural hash.
+
+Parameters
+--
+value
+Input value to hash.
+
+map_free_vars
+Whether free variables mapped to each oth
Re: [PR] [EXTRA] Introduce StructuralKey [tvm-ffi]
tqchen commented on PR #453: URL: https://github.com/apache/tvm-ffi/pull/453#issuecomment-3906193984 /gemini review -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] [EXTRA] Introduce StructuralKey [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #453:
URL: https://github.com/apache/tvm-ffi/pull/453#discussion_r2810450038
##
include/tvm/ffi/extra/structural_key.h:
##
@@ -0,0 +1,90 @@
+/*
+ * 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.
+ */
+/*!
+ * \file tvm/ffi/extra/structural_key.h
+ * \brief Structural-key wrapper that caches structural hash.
+ */
+#ifndef TVM_FFI_EXTRA_STRUCTURAL_KEY_H_
+#define TVM_FFI_EXTRA_STRUCTURAL_KEY_H_
+
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/*!
+ * \brief Object node that contains a key and its cached structural hash.
+ */
+class StructuralKeyObj : public Object {
+ public:
+ /*! \brief The key value. */
+ Any key;
+ /*! \brief Cached structural hash of \p key, encoded as int64_t. */
+ int64_t hash_i64;
+
+ // Default constructor to support reflection-based initialization.
+ StructuralKeyObj() = default;
+ explicit StructuralKeyObj(Any key)
+ : key(std::move(key)),
hash_i64(static_cast(StructuralHash::Hash(this->key))) {}
Review Comment:
The `StructuralKeyObj` constructor computes the structural hash using
default arguments for `StructuralHash::Hash`, which means
`skip_tensor_content=false`. However, both `StructuralKey::operator==` (line
72) and `ffi::StructuralKeyEqual` (in `src/ffi/extra/reflection_extra.cc`) use
`StructuralEqual::Equal` with `skip_tensor_content=true`.
This inconsistency violates the contract that if two objects are equal,
their hashes must also be equal. For two `StructuralKey` objects wrapping
tensors with the same shape but different content, the equality comparison
would consider them equal (since `skip_tensor_content=true`), but their cached
hashes would be different. This would lead to incorrect behavior in hash-based
containers like `std::unordered_map` or `tvm::Map`.
To fix this, the hash should be computed with the same `skip_tensor_content`
policy as the equality check.
```suggestion
: key(std::move(key)),
hash_i64(static_cast(StructuralHash::Hash(this->key,
/*map_free_vars=*/false, /*skip_tensor_content=*/true))) {}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] [EXTRA] Introduce StructuralKey [tvm-ffi]
gemini-code-assist[bot] commented on PR #453: URL: https://github.com/apache/tvm-ffi/pull/453#issuecomment-3906172584 ## Summary of Changes Hello @tqchen, I'm Gemini Code Assist[^1]! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the `StructuralKey` wrapper class, which provides a convenient mechanism to enforce structural equality and hashing for arbitrary `Any` objects. By caching the structural hash and overriding comparison operators, `StructuralKey` enables the use of complex data structures as keys in hash-based containers, such as `std::unordered_map` in C++ or Python dictionaries, where comparison is based on content rather than object identity. This enhancement improves the flexibility and utility of the FFI system for managing and comparing objects with deep structural semantics. ### Highlights * **New StructuralKey Class**: Introduced a new C++ `StructuralKey` class and its `ObjectRef` wrapper, designed to encapsulate an `Any` type and cache its structural hash, facilitating structural equality comparisons. * **Structural Equality and Hashing**: Implemented `operator==` for `StructuralKey` to perform structural equality checks and provided a `std::hash` specialization, enabling `StructuralKey` to be used as keys in C++ standard hash-based containers like `std::unordered_map`. * **Python Bindings and Helpers**: Exposed `StructuralKey` and related structural equality/hashing functions (`structural_equal`, `structural_hash`, `get_first_structural_mismatch`) to the Python `tvm_ffi` module, allowing Python users to leverage structural semantics for object comparison and hashing. * **Reflection and FFI Integration**: Integrated `StructuralKey` into the FFI reflection system, registering its object definition and type attributes, and exposing its constructor and equality comparison through FFI APIs for seamless C++ and Python interoperability. * **Comprehensive Testing**: Added dedicated C++ and Python unit tests to validate the correctness of `StructuralKey`'s equality, hashing, and its behavior when used in both C++ `std::unordered_map` and Python dictionaries/`tvm_ffi.Map`. 🧠**New Feature in Public Preview:** You can now enable **Memory** to help **Gemini Code Assist** learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. **Click [here](https://codeassist.google/code-review/login) to enable Memory in your admin console.** Changelog * **include/tvm/ffi/extra/structural_key.h** * Added `StructuralKeyObj` and `StructuralKey` classes for structural hashing and equality. * Implemented `operator==` for `StructuralKey` to perform structural comparisons. * Provided a `std::hash` specialization for `StructuralKey` to enable its use in hash tables. * **python/tvm_ffi/__init__.py** * Imported `StructuralKey` and structural helper functions (`get_first_structural_mismatch`, `structural_equal`, `structural_hash`). * Added `StructuralKey` and structural helper functions to the module's `__all__` export list. * **python/tvm_ffi/_ffi_api.py** * Added FFI definitions for `StructuralKey`, `StructuralKeyEqual`, `StructuralKeyHash`, and `StructuralEqual`. * **python/tvm_ffi/structural.py** * Added new file defining Python functions `structural_equal`, `structural_hash`, and `get_first_structural_mismatch`. * Implemented the Python `StructuralKey` class with `__init__`, `__hash__`, and `__eq__` methods, leveraging C++ FFI. * **src/ffi/extra/reflection_extra.cc** * Included `tvm/ffi/extra/structural_key.h`. * Registered `StructuralKeyObj` for reflection, exposing its `key` and `hash_i64` members. * Defined `StructuralKeyHash` and `StructuralKeyEqual` functions for FFI. * Exposed `ffi.StructuralKey` constructor and `ffi.StructuralKeyEqual` via FFI. * **src/ffi/extra/structural_equal.cc** * Exposed `ffi.StructuralEqual` via FFI. * **tests/cpp/extra/test_structural_key.cc** * Added new C++ unit tests for `StructuralKey` covering equality, `std::hash` behavior, and usage in `std::unordered_map`. * **tests/python/test_structural.py** * Added new Python unit tests for `StructuralKey` and structural helper functions. * Verified `StructuralKey` behavior in Python dictionaries and `tvm_ffi.Map`. Using Gemini Code Assist The full guide for Gemini Code Assist can be found on our [documentation page](https://developers.google.com/gemini-code-assist/docs/review-github-code), here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either `/gemini ` or `@gemini-code-assist `
