Re: [PR] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
junrushao merged PR #438: URL: https://github.com/apache/tvm-ffi/pull/438 -- 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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
tqchen commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2806752339
##
include/tvm/ffi/reflection/registry.h:
##
@@ -460,6 +460,9 @@ struct init {
}
};
+/*! \brief Method name used for shallow copy registration. */
+inline constexpr const char* kShallowCopyMethodName = "__ffi_shallow_copy__";
Review Comment:
kInitMethodName is member of ObjectDef, for consistency maybe make it also
member?
Alterntaively, consider group these constants into struct or namespace like
`struct StaticTypeKey {`. Maybe TypeAttrName or `reflection::type_attr::`
--
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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
tqchen commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2806747880
##
src/ffi/extra/deep_copy.cc:
##
@@ -0,0 +1,162 @@
+/*
+ * 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 src/ffi/extra/deep_copy.cc
+ *
+ * \brief Reflection-based deep copy utilities.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/*!
+ * \brief Deep copier with memoization.
+ *
+ * - Arrays / Maps: Resolve() recurses to rebuild with resolved children.
+ * - Copyable objects: shallow-copied immediately into copy_map_ (so cyclic
+ * back-references resolve), then queued for field resolution.
+ * - The queue is drained iteratively by Run(), bounding recursion depth
+ * to container nesting rather than object-graph depth.
+ * - Shared references are preserved: the same original maps to the same copy.
+ */
+class ObjectDeepCopier {
+ public:
+ explicit ObjectDeepCopier(reflection::TypeAttrColumn* column) :
column_(column) {}
+
+ Any Run(const Any& value) {
+if (value.type_index() < TypeIndex::kTVMFFIStaticObjectBegin) return value;
+Any result = Resolve(value);
+// NOLINTNEXTLINE(modernize-loop-convert): queue grows during iteration
+for (size_t i = 0; i < resolve_queue_.size(); ++i) {
+ ResolveFields(resolve_queue_[i]);
+}
+return result;
+ }
+
+ private:
+ /*! \brief Resolve a value: pass through primitives, copy/rebuild objects. */
+ Any Resolve(const Any& value) {
+if (value.type_index() < TypeIndex::kTVMFFIStaticObjectBegin) {
+ return value;
+}
+const Object* obj = value.as();
+if (auto it = copy_map_.find(obj); it != copy_map_.end()) {
+ return it->second;
+}
+int32_t ti = obj->type_index();
+// Strings and bytes are immutable — return as-is.
+if (ti == TypeIndex::kTVMFFIStr || ti == TypeIndex::kTVMFFIBytes) {
Review Comment:
this also applies to kTVMFFIShape which is immutable and can return as it is
--
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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2792040790
##
include/tvm/ffi/extra/deep_copy.h:
##
@@ -0,0 +1,48 @@
+/*
+ * 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/deep_copy.h
+ * \brief Reflection-based object copy utilities
+ */
+#ifndef TVM_FFI_EXTRA_DEEP_COPY_H_
+#define TVM_FFI_EXTRA_DEEP_COPY_H_
+
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/**
+ * \brief Deep copy an ffi::Any value.
+ *
+ * Recursively copies the value and all reachable objects in its object graph.
+ * Copy-constructible types with `ObjectDef` registration automatically
support deep copy.
+ * Primitive types, strings, and bytes are returned as-is (they are immutable).
+ * Arrays and Maps are recursively deep copied.
+ * Objects without copy support are kept as shared references.
Review Comment:
The documentation states that objects without copy support are kept as
shared references. However, the implementation in `src/ffi/extra/deep_copy.cc`
throws an error if an object is not copy-constructible. This is a significant
discrepancy. The implementation's behavior of throwing an error seems more
appropriate for a deep copy operation, as silently falling back to a shallow
copy could lead to unexpected behavior. Please update the documentation to
reflect that an error is thrown for non-copyable objects.
```c
* Objects that are not copy-constructible will cause a runtime error.
```
--
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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
junrushao commented on PR #438: URL: https://github.com/apache/tvm-ffi/pull/438#issuecomment-3883040511 /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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
junrushao commented on PR #438: URL: https://github.com/apache/tvm-ffi/pull/438#issuecomment-3882928238 Updated per discussion with TQ. @tqchen please re-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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
junrushao commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2791512956
##
include/tvm/ffi/extra/copy.h:
##
@@ -0,0 +1,48 @@
+/*
+ * 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/copy.h
+ * \brief Reflection-based object copy utilities
+ */
+#ifndef TVM_FFI_EXTRA_COPY_H_
+#define TVM_FFI_EXTRA_COPY_H_
+
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/**
+ * \brief Deep copy an ffi::Any value.
+ *
+ * Recursively copies the value and all reachable objects in its object graph.
+ * Objects must have copy support enabled via `refl::enable_copy()` to be deep
copied.
+ * Primitive types, strings, and bytes are returned as-is (they are immutable).
+ * Arrays and Maps are recursively deep copied.
+ * Objects without copy support are kept as shared references.
+ *
+ * \param value The value to deep copy.
+ * \return The deep copied value.
+ */
+TVM_FFI_EXTRA_CXX_API Any DeepCopyValue(const Any& value);
Review Comment:
good idea!
##
include/tvm/ffi/extra/copy.h:
##
@@ -0,0 +1,48 @@
+/*
+ * 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/copy.h
+ * \brief Reflection-based object copy utilities
+ */
+#ifndef TVM_FFI_EXTRA_COPY_H_
+#define TVM_FFI_EXTRA_COPY_H_
Review Comment:
updated
--
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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
junrushao commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2791488179
##
include/tvm/ffi/reflection/registry.h:
##
@@ -460,6 +460,49 @@ struct init {
}
};
+/*!
+ * \brief Helper class to enable copy support for object types.
+ *
+ * This helper is used with `ObjectDef::def()` to register
`__ffi_shallow_copy__`
+ * as an instance method. This enables `__copy__`, `__deepcopy__`, and
`__replace__`
+ * in Python via the `copy` module.
+ *
+ * The shallow copy uses the C++ copy constructor of the object type:
+ * `make_object(*self)`.
+ *
+ * Example usage:
+ *
+ * \code{.cpp}
+ * refl::ObjectDef()
+ * .def(refl::enable_copy())
+ * .def(refl::init());
+ * \endcode
+ *
+ * \note The object type must have a copy constructor.
+ */
+struct enable_copy {
Review Comment:
alternatively, let's enable copy by default for all object types which are
copy_constructible
--
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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
junrushao commented on PR #438: URL: https://github.com/apache/tvm-ffi/pull/438#issuecomment-3874675284 Still - implication of cyclic reference and CoW containers (Array, Map) makes it pretty challenging to have an absolutely correct implementation of `__deepcopy__`. Converting it to draft. -- 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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
tqchen commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2784035440
##
include/tvm/ffi/reflection/registry.h:
##
@@ -460,6 +460,49 @@ struct init {
}
};
+/*!
+ * \brief Helper class to enable copy support for object types.
+ *
+ * This helper is used with `ObjectDef::def()` to register
`__ffi_shallow_copy__`
+ * as an instance method. This enables `__copy__`, `__deepcopy__`, and
`__replace__`
+ * in Python via the `copy` module.
+ *
+ * The shallow copy uses the C++ copy constructor of the object type:
+ * `make_object(*self)`.
+ *
+ * Example usage:
+ *
+ * \code{.cpp}
+ * refl::ObjectDef()
+ * .def(refl::enable_copy())
+ * .def(refl::init());
+ * \endcode
+ *
+ * \note The object type must have a copy constructor.
+ */
+struct enable_copy {
Review Comment:
yes, i understand, seems the confusion part is that it enables a shallow
copy API that was required for deep copy?
--
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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
junrushao commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2783983437
##
include/tvm/ffi/reflection/registry.h:
##
@@ -460,6 +460,49 @@ struct init {
}
};
+/*!
+ * \brief Helper class to enable copy support for object types.
+ *
+ * This helper is used with `ObjectDef::def()` to register
`__ffi_shallow_copy__`
+ * as an instance method. This enables `__copy__`, `__deepcopy__`, and
`__replace__`
+ * in Python via the `copy` module.
+ *
+ * The shallow copy uses the C++ copy constructor of the object type:
+ * `make_object(*self)`.
+ *
+ * Example usage:
+ *
+ * \code{.cpp}
+ * refl::ObjectDef()
+ * .def(refl::enable_copy())
+ * .def(refl::init());
+ * \endcode
+ *
+ * \note The object type must have a copy constructor.
+ */
+struct enable_copy {
Review Comment:
No it's for shallow copy
--
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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
tqchen commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2782403728
##
include/tvm/ffi/extra/copy.h:
##
@@ -0,0 +1,48 @@
+/*
+ * 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/copy.h
+ * \brief Reflection-based object copy utilities
+ */
+#ifndef TVM_FFI_EXTRA_COPY_H_
+#define TVM_FFI_EXTRA_COPY_H_
+
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/**
+ * \brief Deep copy an ffi::Any value.
+ *
+ * Recursively copies the value and all reachable objects in its object graph.
+ * Objects must have copy support enabled via `refl::enable_copy()` to be deep
copied.
+ * Primitive types, strings, and bytes are returned as-is (they are immutable).
+ * Arrays and Maps are recursively deep copied.
+ * Objects without copy support are kept as shared references.
+ *
+ * \param value The value to deep copy.
+ * \return The deep copied value.
+ */
+TVM_FFI_EXTRA_CXX_API Any DeepCopyValue(const Any& value);
Review Comment:
I feel DeepCopy as an API name is fine here
##
include/tvm/ffi/extra/copy.h:
##
@@ -0,0 +1,48 @@
+/*
+ * 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/copy.h
+ * \brief Reflection-based object copy utilities
+ */
+#ifndef TVM_FFI_EXTRA_COPY_H_
+#define TVM_FFI_EXTRA_COPY_H_
Review Comment:
deep_copy.h?
##
include/tvm/ffi/reflection/registry.h:
##
@@ -460,6 +460,49 @@ struct init {
}
};
+/*!
+ * \brief Helper class to enable copy support for object types.
+ *
+ * This helper is used with `ObjectDef::def()` to register
`__ffi_shallow_copy__`
+ * as an instance method. This enables `__copy__`, `__deepcopy__`, and
`__replace__`
+ * in Python via the `copy` module.
+ *
+ * The shallow copy uses the C++ copy constructor of the object type:
+ * `make_object(*self)`.
+ *
+ * Example usage:
+ *
+ * \code{.cpp}
+ * refl::ObjectDef()
+ * .def(refl::enable_copy())
+ * .def(refl::init());
+ * \endcode
+ *
+ * \note The object type must have a copy constructor.
+ */
+struct enable_copy {
Review Comment:
should it be enable_deep_copy to be explicit?
##
src/ffi/extra/copy.cc:
##
@@ -0,0 +1,268 @@
+/*
+ * 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 src/ffi/extra/copy.cc
+ *
+ * \brief Reflection-based deep copy utilities.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/*!
+ * \brief Deep copier that recursively copies objects using shallow copy +
field replacement.
+ *
+ * Algorithm:
+ * Phase 1: Walk t
Re: [PR] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2780989838
##
include/tvm/ffi/reflection/registry.h:
##
@@ -587,6 +630,42 @@ class ObjectDef : public ReflectionDefBase {
return *this;
}
+ /*!
+ * \brief Enable copy support for this object type.
+ *
+ * Registers `__ffi_shallow_copy__` as an instance method that performs a
shallow
+ * copy via the C++ copy constructor: `make_object(*self)`.
+ * Also registers it as a type attribute so that the generic deep copy
function
+ * can look it up by type index.
+ *
+ * \param ec An instance of `enable_copy`.
+ * \param extra Optional additional metadata such as docstring.
+ *
+ * \return Reference to this `ObjectDef` for method chaining.
+ *
+ * Example:
+ *
+ * \code{.cpp}
+ * refl::ObjectDef()
+ * .def(refl::enable_copy())
+ * .def(refl::init());
+ * \endcode
+ */
+ template
+ TVM_FFI_INLINE ObjectDef& def([[maybe_unused]] enable_copy ec, Extra&&...
extra) {
+// Register __ffi_shallow_copy__ as an instance method
+RegisterMethod(kShallowCopyMethodName, false, &enable_copy::template
shallow_copy,
+ std::forward(extra)...);
+// Also register as a type attribute for generic deep copy lookup
+Function copy_fn = GetMethod(std::string(type_key_) + "." +
kShallowCopyMethodName,
+ &enable_copy::template shallow_copy);
+TVMFFIByteArray attr_name = {kShallowCopyMethodName,
+
std::char_traits::length(kShallowCopyMethodName)};
+TVMFFIAny attr_value = AnyView(copy_fn).CopyToTVMFFIAny();
+TVM_FFI_CHECK_SAFE_CALL(TVMFFITypeRegisterAttr(type_index_, &attr_name,
&attr_value));
+return *this;
+ }
Review Comment:
The current implementation creates the `Function` object for the shallow
copy method twice: once within `RegisterMethod` on line 657 and again
explicitly via `GetMethod` on line 660. This is slightly inefficient as it
involves redundant work during type registration.
You can avoid this by retrieving the method that was just registered using
`TVMFFITypeGetMethod` and then using that to register the type attribute. This
makes the code more efficient and avoids the redundant object creation.
```c
TVM_FFI_INLINE ObjectDef& def([[maybe_unused]] enable_copy ec, Extra&&...
extra) {
// Register __ffi_shallow_copy__ as an instance method
RegisterMethod(kShallowCopyMethodName, false, &enable_copy::template
shallow_copy,
std::forward(extra)...);
// Also register as a type attribute for generic deep copy lookup
TVMFFIMethodInfo method_info;
TVMFFIByteArray method_name = {kShallowCopyMethodName,
std::char_traits::length(kShallowCopyMethodName)};
TVM_FFI_CHECK_SAFE_CALL(TVMFFITypeGetMethod(type_index_, &method_name,
&method_info));
TVM_FFI_CHECK_SAFE_CALL(TVMFFITypeRegisterAttr(type_index_,
&method_name, &method_info.method));
return *this;
}
```
--
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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
junrushao commented on PR #438: URL: https://github.com/apache/tvm-ffi/pull/438#issuecomment-3869773464 /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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2780848789
##
src/ffi/extra/copy.cc:
##
@@ -0,0 +1,293 @@
+/*
+ * 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 src/ffi/extra/copy.cc
+ *
+ * \brief Reflection-based deep copy utilities.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/*!
+ * \brief Deep copier that recursively copies objects using shallow copy +
field replacement.
+ *
+ * Algorithm:
+ * Phase 1: Walk the entire object graph (objects, arrays, maps) using an
explicit
+ * stack, collecting all reachable nodes in topological order
(post-order DFS).
+ * An unordered_set deduplicates nodes, preserving shared references.
+ * Phase 2: In two sub-passes:
+ * (a) Create shallow copies of all copyable objects first, so that
+ * back-references in cyclic object graphs resolve correctly.
+ * (b) Create deep-copied arrays/maps with resolved elements, and
+ * resolve fields of copyable objects with previously copied
values.
+ *
+ * Primitive types, strings, bytes, and functions are immutable and returned
as-is.
+ * Arrays and Maps are recursively copied (their elements are replaced with
copies).
+ * Shared references (including shared containers) are preserved in the copy
graph.
+ * Objects without enable_copy() are kept as shared references.
+ */
+class ObjectDeepCopier {
+ public:
+ static Any DeepCopy(const Any& root) {
+ObjectDeepCopier copier;
+// Phase 1: Collect all reachable nodes in topological order
+copier.CollectNodes(root);
+// Phase 2: Copy nodes in topological order
+copier.CopyNodesInOrder();
+// Phase 3: Resolve the root value with copies
+return copier.ResolveValue(root);
+ }
+
+ private:
+ struct DFSFrame {
+const Object* obj;
+bool children_pushed;
+ };
+
+ // The type attribute column for looking up per-type shallow copy functions
+ static reflection::TypeAttrColumn& ShallowCopyColumn() {
+static reflection::TypeAttrColumn column("__ffi_shallow_copy__");
+return column;
+ }
+
+ // Map from original object pointer to its copy
+ std::unordered_map copy_map_;
+ // Topological order of nodes to copy (post-order, leaves first)
+ std::vector topo_order_;
+ // Set of visited object pointers for deduplication
+ std::unordered_set visited_;
+ // Original Any values keyed by Object pointer (for typed container access)
+ std::unordered_map originals_;
+
+ /*!
+ * \brief Extract the Object pointer from an Any value.
+ * \return The Object pointer, or nullptr if the value is not an
object/array/map.
+ */
+ static const Object* AsObjectPtr(const Any& value) {
+int32_t ti = value.type_index();
+if (ti == TypeIndex::kTVMFFIArray || ti == TypeIndex::kTVMFFIMap ||
+ti >= TypeIndex::kTVMFFIStaticObjectBegin) {
+ return details::AnyUnsafe::CopyFromAnyViewAfterCheck(value);
+}
+return nullptr;
+ }
+
+ /*!
+ * \brief Phase 1: Walk the object graph iteratively, collecting all
reachable nodes
+ *(objects, arrays, maps) in topological order (post-order DFS).
+ */
+ void CollectNodes(const Any& root) {
+const Object* root_obj = AsObjectPtr(root);
+if (root_obj == nullptr) return;
+originals_.emplace(root_obj, root);
+
+std::stack stack;
+stack.push({root_obj, false});
+
+while (!stack.empty()) {
+ DFSFrame& frame = stack.top();
+
+ if (frame.children_pushed) {
+// All children processed, add this node in post-order
+topo_order_.push_back(frame.obj);
+stack.pop();
+continue;
+ }
+
+ frame.children_pushed = true;
+
+ // Dedup at process time (not push time) to ensure correct topological
order
+ // when multiple parents share the same child node.
+ if (!visited_.insert(frame.obj).second) {
+stack.pop();
+continue;
+ }
+
+ int32_t ti = frame.obj->type_index();
+
+ if (ti == TypeIndex::kTVMFFIArray) {
+// Push array elements
Re: [PR] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2780848789
##
src/ffi/extra/copy.cc:
##
@@ -0,0 +1,293 @@
+/*
+ * 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 src/ffi/extra/copy.cc
+ *
+ * \brief Reflection-based deep copy utilities.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/*!
+ * \brief Deep copier that recursively copies objects using shallow copy +
field replacement.
+ *
+ * Algorithm:
+ * Phase 1: Walk the entire object graph (objects, arrays, maps) using an
explicit
+ * stack, collecting all reachable nodes in topological order
(post-order DFS).
+ * An unordered_set deduplicates nodes, preserving shared references.
+ * Phase 2: In two sub-passes:
+ * (a) Create shallow copies of all copyable objects first, so that
+ * back-references in cyclic object graphs resolve correctly.
+ * (b) Create deep-copied arrays/maps with resolved elements, and
+ * resolve fields of copyable objects with previously copied
values.
+ *
+ * Primitive types, strings, bytes, and functions are immutable and returned
as-is.
+ * Arrays and Maps are recursively copied (their elements are replaced with
copies).
+ * Shared references (including shared containers) are preserved in the copy
graph.
+ * Objects without enable_copy() are kept as shared references.
+ */
+class ObjectDeepCopier {
+ public:
+ static Any DeepCopy(const Any& root) {
+ObjectDeepCopier copier;
+// Phase 1: Collect all reachable nodes in topological order
+copier.CollectNodes(root);
+// Phase 2: Copy nodes in topological order
+copier.CopyNodesInOrder();
+// Phase 3: Resolve the root value with copies
+return copier.ResolveValue(root);
+ }
+
+ private:
+ struct DFSFrame {
+const Object* obj;
+bool children_pushed;
+ };
+
+ // The type attribute column for looking up per-type shallow copy functions
+ static reflection::TypeAttrColumn& ShallowCopyColumn() {
+static reflection::TypeAttrColumn column("__ffi_shallow_copy__");
+return column;
+ }
+
+ // Map from original object pointer to its copy
+ std::unordered_map copy_map_;
+ // Topological order of nodes to copy (post-order, leaves first)
+ std::vector topo_order_;
+ // Set of visited object pointers for deduplication
+ std::unordered_set visited_;
+ // Original Any values keyed by Object pointer (for typed container access)
+ std::unordered_map originals_;
+
+ /*!
+ * \brief Extract the Object pointer from an Any value.
+ * \return The Object pointer, or nullptr if the value is not an
object/array/map.
+ */
+ static const Object* AsObjectPtr(const Any& value) {
+int32_t ti = value.type_index();
+if (ti == TypeIndex::kTVMFFIArray || ti == TypeIndex::kTVMFFIMap ||
+ti >= TypeIndex::kTVMFFIStaticObjectBegin) {
+ return details::AnyUnsafe::CopyFromAnyViewAfterCheck(value);
+}
+return nullptr;
+ }
+
+ /*!
+ * \brief Phase 1: Walk the object graph iteratively, collecting all
reachable nodes
+ *(objects, arrays, maps) in topological order (post-order DFS).
+ */
+ void CollectNodes(const Any& root) {
+const Object* root_obj = AsObjectPtr(root);
+if (root_obj == nullptr) return;
+originals_.emplace(root_obj, root);
+
+std::stack stack;
+stack.push({root_obj, false});
+
+while (!stack.empty()) {
+ DFSFrame& frame = stack.top();
+
+ if (frame.children_pushed) {
+// All children processed, add this node in post-order
+topo_order_.push_back(frame.obj);
+stack.pop();
+continue;
+ }
+
+ frame.children_pushed = true;
+
+ // Dedup at process time (not push time) to ensure correct topological
order
+ // when multiple parents share the same child node.
+ if (!visited_.insert(frame.obj).second) {
+stack.pop();
+continue;
+ }
+
+ int32_t ti = frame.obj->type_index();
+
+ if (ti == TypeIndex::kTVMFFIArray) {
+// Push array elements
Re: [PR] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2780848777
##
src/ffi/extra/copy.cc:
##
@@ -0,0 +1,293 @@
+/*
+ * 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 src/ffi/extra/copy.cc
+ *
+ * \brief Reflection-based deep copy utilities.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/*!
+ * \brief Deep copier that recursively copies objects using shallow copy +
field replacement.
+ *
+ * Algorithm:
+ * Phase 1: Walk the entire object graph (objects, arrays, maps) using an
explicit
+ * stack, collecting all reachable nodes in topological order
(post-order DFS).
+ * An unordered_set deduplicates nodes, preserving shared references.
+ * Phase 2: In two sub-passes:
+ * (a) Create shallow copies of all copyable objects first, so that
+ * back-references in cyclic object graphs resolve correctly.
+ * (b) Create deep-copied arrays/maps with resolved elements, and
+ * resolve fields of copyable objects with previously copied
values.
Review Comment:
The documentation for Phase 2 of the deep copy algorithm mentions two
sub-passes, but the implementation in `CopyNodesInOrder` actually uses three
distinct sub-passes. The comment within `CopyNodesInOrder` correctly describes
these three passes. To improve clarity and consistency, the main class-level
documentation should be updated to reflect the three-pass approach.
```suggestion
* Phase 2: In three sub-passes:
* (a) Create shallow copies of all copyable objects. This allows
* back-references in cyclic graphs to be resolved correctly.
* (b) Create deep copies of arrays and maps. Their elements are
* resolved to their copied versions from sub-pass (a).
* (c) Resolve fields of the shallow-copied objects from (a) to
point
* to their deep-copied versions from (a) and (b).
```
##
src/ffi/extra/copy.cc:
##
@@ -0,0 +1,293 @@
+/*
+ * 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 src/ffi/extra/copy.cc
+ *
+ * \brief Reflection-based deep copy utilities.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/*!
+ * \brief Deep copier that recursively copies objects using shallow copy +
field replacement.
+ *
+ * Algorithm:
+ * Phase 1: Walk the entire object graph (objects, arrays, maps) using an
explicit
+ * stack, collecting all reachable nodes in topological order
(post-order DFS).
+ * An unordered_set deduplicates nodes, preserving shared references.
+ * Phase 2: In two sub-passes:
+ * (a) Create shallow copies of all copyable objects first, so that
+ * back-references in cyclic object graphs resolve correctly.
+ * (b) Create deep-copied arrays/maps with resolved elements, and
+ * resolve fields of copyable objects with previously copied
values.
+ *
+ * Primitive types, strings, bytes, and functions are immutable and returned
as-is.
+ * Arrays and Maps are recursi
Re: [PR] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
junrushao commented on PR #438: URL: https://github.com/apache/tvm-ffi/pull/438#issuecomment-3869613919 /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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
junrushao commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2780822687
##
python/tvm_ffi/registry.py:
##
@@ -335,25 +335,97 @@ def _add_class_attrs(type_cls: type, type_info: TypeInfo)
-> type:
if not hasattr(type_cls, name): # skip already defined attributes
setattr(type_cls, name, field.as_property(type_cls))
has_c_init = False
+has_shallow_copy = False
for method in type_info.methods:
name = method.name
if name == "__ffi_init__":
name = "__c_ffi_init__"
has_c_init = True
-if not hasattr(type_cls, name):
+if name == "__ffi_shallow_copy__":
+has_shallow_copy = True
+# Always override: shallow copy is type-specific and must not be
inherited
+setattr(type_cls, name, method.as_callable(type_cls))
+elif not hasattr(type_cls, name):
setattr(type_cls, name, method.as_callable(type_cls))
if "__init__" not in type_cls.__dict__:
if has_c_init:
setattr(type_cls, "__init__", getattr(type_cls, "__ffi_init__"))
elif not issubclass(type_cls, core.PyNativeObject):
setattr(type_cls, "__init__", __init__invalid)
+_setup_copy_methods(type_cls, has_shallow_copy)
return type_cls
+def _setup_copy_methods(type_cls: type, has_shallow_copy: bool) -> None:
+"""Set up __copy__, __deepcopy__, __replace__ based on copy support."""
+if has_shallow_copy:
+if "__copy__" not in type_cls.__dict__:
+setattr(type_cls, "__copy__", _copy_supported)
+if "__deepcopy__" not in type_cls.__dict__:
+setattr(type_cls, "__deepcopy__", _deepcopy_supported)
+if "__replace__" not in type_cls.__dict__:
+setattr(type_cls, "__replace__", _replace_supported)
+else:
+if "__copy__" not in type_cls.__dict__:
+setattr(type_cls, "__copy__", _copy_unsupported)
+if "__deepcopy__" not in type_cls.__dict__:
+setattr(type_cls, "__deepcopy__", _deepcopy_unsupported)
+if "__replace__" not in type_cls.__dict__:
+setattr(type_cls, "__replace__", _replace_unsupported)
+
+
def __init__invalid(self: Any, *args: Any, **kwargs: Any) -> None:
raise RuntimeError("The __init__ method of this class is not implemented.")
+def _copy_supported(self: Any) -> Any:
+return self.__ffi_shallow_copy__()
+
+
+def _deepcopy_supported(self: Any, memo: Any = None) -> Any:
+return _get_deep_copy_func()(self)
+
+
+def _replace_supported(self: Any, **kwargs: Any) -> Any:
+import copy # noqa: PLC0415
+
+obj = copy.copy(self)
+for key, value in kwargs.items():
+setattr(obj, key, value)
+return obj
+
+
+def _copy_unsupported(self: Any) -> Any:
+raise TypeError(
+f"Type `{type(self).__name__}` does not support copy. "
+f"Enable it with refl::enable_copy() in the C++ type registration."
+)
+
+
+def _deepcopy_unsupported(self: Any, memo: Any = None) -> Any:
+raise TypeError(
+f"Type `{type(self).__name__}` does not support deepcopy. "
+f"Enable it with refl::enable_copy() in the C++ type registration."
+)
+
+
+def _replace_unsupported(self: Any, **kwargs: Any) -> Any:
+raise TypeError(
+f"Type `{type(self).__name__}` does not support replace. "
+f"Enable it with refl::enable_copy() in the C++ type registration."
+)
+
+
+def _get_deep_copy_func() -> core.Function:
+global _deep_copy_func_cache # noqa: PLW0603
+if _deep_copy_func_cache is None:
+_deep_copy_func_cache = get_global_func("ffi.DeepCopy")
+return _deep_copy_func_cache
+
+
+_deep_copy_func_cache: core.Function | None = None
Review Comment:
probably don't need this - just call `get_global_func` every time
--
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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2780743716
##
src/ffi/extra/copy.cc:
##
@@ -0,0 +1,275 @@
+/*
+ * 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 src/ffi/extra/copy.cc
+ *
+ * \brief Reflection-based deep copy utilities.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/*!
+ * \brief Deep copier that recursively copies objects using shallow copy +
field replacement.
+ *
+ * Algorithm:
+ * Phase 1: Walk the entire object graph (objects, arrays, maps) using an
explicit
+ * stack, collecting all reachable nodes in topological order
(post-order DFS).
+ * An unordered_set deduplicates nodes, preserving shared references.
+ * Phase 2: In two sub-passes:
+ * (a) Create shallow copies of all copyable objects first, so that
+ * back-references in cyclic object graphs resolve correctly.
+ * (b) Create deep-copied arrays/maps with resolved elements, and
+ * resolve fields of copyable objects with previously copied
values.
+ *
+ * Primitive types, strings, bytes, and functions are immutable and returned
as-is.
+ * Arrays and Maps are recursively copied (their elements are replaced with
copies).
+ * Shared references (including shared containers) are preserved in the copy
graph.
+ * Objects without enable_copy() are kept as shared references.
+ */
+class ObjectDeepCopier {
+ public:
+ static Any DeepCopy(const Any& root) {
+ObjectDeepCopier copier;
+// Phase 1: Collect all reachable nodes in topological order
+copier.CollectNodes(root);
+// Phase 2: Copy nodes in topological order
+copier.CopyNodesInOrder();
+// Phase 3: Resolve the root value with copies
+return copier.ResolveValue(root);
+ }
+
+ private:
+ struct DFSFrame {
+const Object* obj;
+bool children_pushed;
+ };
+
+ // The type attribute column for looking up per-type shallow copy functions
+ static reflection::TypeAttrColumn& ShallowCopyColumn() {
+static reflection::TypeAttrColumn column("__ffi_shallow_copy__");
+return column;
+ }
+
+ // Map from original object pointer to its copy
+ std::unordered_map copy_map_;
+ // Topological order of nodes to copy (post-order, leaves first)
+ std::vector topo_order_;
+ // Set of visited object pointers for deduplication
+ std::unordered_set visited_;
+ // Original Any values keyed by Object pointer (for typed container access)
+ std::unordered_map originals_;
+
+ /*!
+ * \brief Extract the Object pointer from an Any value.
+ * \return The Object pointer, or nullptr if the value is not an
object/array/map.
+ */
+ static const Object* AsObjectPtr(const Any& value) {
+int32_t ti = value.type_index();
+if (ti == TypeIndex::kTVMFFIArray || ti == TypeIndex::kTVMFFIMap ||
+ti >= TypeIndex::kTVMFFIStaticObjectBegin) {
+ return details::AnyUnsafe::CopyFromAnyViewAfterCheck(value);
+}
+return nullptr;
+ }
+
+ /*!
+ * \brief Phase 1: Walk the object graph iteratively, collecting all
reachable nodes
+ *(objects, arrays, maps) in topological order (post-order DFS).
+ */
+ void CollectNodes(const Any& root) {
+const Object* root_obj = AsObjectPtr(root);
+if (root_obj == nullptr) return;
+originals_.emplace(root_obj, root);
+
+std::stack stack;
+stack.push({root_obj, false});
+
+while (!stack.empty()) {
+ DFSFrame& frame = stack.top();
+
+ if (frame.children_pushed) {
+// All children processed, add this node in post-order
+topo_order_.push_back(frame.obj);
+stack.pop();
+continue;
+ }
+
+ frame.children_pushed = true;
+
+ // Dedup at process time (not push time) to ensure correct topological
order
+ // when multiple parents share the same child node.
+ if (!visited_.insert(frame.obj).second) {
+stack.pop();
+continue;
+ }
+
+ int32_t ti = frame.obj->type_index();
+
+ if (ti == TypeIndex::kTVMFFIArray) {
+// Push array elements
Re: [PR] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
junrushao commented on PR #438: URL: https://github.com/apache/tvm-ffi/pull/438#issuecomment-3869480545 /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] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
gemini-code-assist[bot] commented on code in PR #438:
URL: https://github.com/apache/tvm-ffi/pull/438#discussion_r2780540797
##
src/ffi/extra/copy.cc:
##
@@ -0,0 +1,287 @@
+/*
+ * 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 src/ffi/extra/copy.cc
+ *
+ * \brief Reflection-based deep copy utilities.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+namespace tvm {
+namespace ffi {
+
+/*!
+ * \brief Deep copier that recursively copies objects using shallow copy +
field replacement.
+ *
+ * Algorithm:
+ * Phase 1: Walk the object graph using an explicit stack, collecting all
reachable
+ * objects in topological order (post-order DFS). An unordered_set is
used
+ * for deduplication.
+ * Phase 2: In topological order (leaves first), create shallow copies of each
object
+ * and replace their fields with previously copied values.
+ *
+ * Primitive types, strings, bytes, and functions are immutable and returned
as-is.
+ * Arrays and Maps are recursively copied (their elements are replaced with
copies).
+ * Objects without enable_copy() are kept as shared references.
+ */
+class ObjectDeepCopier {
+ public:
+ static Any DeepCopy(const Any& root) {
+ObjectDeepCopier copier;
+// Phase 1: Collect all copyable objects in topological order
+copier.CollectObjects(root);
+// Phase 2: Copy objects in topological order (leaves first)
+copier.CopyObjectsInOrder();
+// Phase 3: Resolve the root value with copies
+return copier.Resolve(root);
+ }
+
+ private:
+ struct DFSFrame {
+const Object* obj;
+bool children_pushed;
+ };
+
+ // The type attribute column for looking up per-type shallow copy functions
+ static reflection::TypeAttrColumn& ShallowCopyColumn() {
+static reflection::TypeAttrColumn column("__ffi_shallow_copy__");
+return column;
+ }
+
+ // Map from original object pointer to its shallow copy
+ std::unordered_map copy_map_;
+ // Topological order of objects to copy (post-order, leaves first)
+ std::vector topo_order_;
+ // Set of visited object pointers for deduplication
+ std::unordered_set visited_;
+
+ /*!
+ * \brief Phase 1: Walk the object graph, collecting copyable objects
+ *in topological order (post-order DFS).
+ */
+ void CollectObjects(const Any& root) { WalkValue(root); }
+
+ /*!
+ * \brief Recursively walk through Any values to find all objects.
+ */
+ void WalkValue(const Any& value) {
+switch (value.type_index()) {
+ case TypeIndex::kTVMFFIArray: {
+Array arr =
details::AnyUnsafe::CopyFromAnyViewAfterCheck>(value);
+for (const Any& elem : arr) {
+ WalkValue(elem);
+}
+break;
+ }
+ case TypeIndex::kTVMFFIMap: {
+Map m =
details::AnyUnsafe::CopyFromAnyViewAfterCheck>(value);
+for (const auto& [k, v] : m) {
+ WalkValue(k);
+ WalkValue(v);
+}
+break;
+ }
+ default: {
+if (value.type_index() >= TypeIndex::kTVMFFIStaticObjectBegin) {
+ const Object* obj =
details::AnyUnsafe::CopyFromAnyViewAfterCheck(value);
+ if (obj != nullptr && visited_.find(obj) == visited_.end()) {
+CollectObjectDFS(obj);
+ }
+}
+break;
+ }
+}
+ }
Review Comment:
The traversal of containers (`Array`, `Map`) in `WalkValue`,
`PushFieldChildren`, and `Resolve` is recursive. This could lead to a stack
overflow for deeply nested containers (e.g., an `Array` of `Array`s).
Since `CollectObjectDFS` already uses an iterative approach with an explicit
stack to handle deep object graphs, I recommend applying a similar iterative
pattern for container traversal here. This would make the deep copy mechanism
fully robust against stack overflows for any object graph structure.
--
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: disc
Re: [PR] feat: add `__copy__`, `__deepcopy__`, and `__replace__` for FFI objects [tvm-ffi]
gemini-code-assist[bot] commented on PR #438: URL: https://github.com/apache/tvm-ffi/pull/438#issuecomment-3869233311 ## Summary of Changes Hello @junrushao, 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 significantly enhances the interoperability between C++ FFI objects and Python by introducing robust copying mechanisms. It allows users to perform shallow and deep copies, as well as field-based replacements, on C++-backed objects directly from Python, aligning their behavior with standard Python objects. This change improves the usability and flexibility of FFI objects within Python workflows. ### Highlights * **C++ Reflection API Enhancement**: Introduced `refl::enable_copy()` in the C++ reflection API to register a `__ffi_shallow_copy__` method, allowing C++ objects to define their shallow copy behavior using their copy constructor. * **Deep Copy Mechanism**: Developed `ObjectDeepCopier` in C++ (`src/ffi/extra/copy.cc`) which performs a two-phase deep copy: first, it walks the object graph using DFS to collect objects in topological order, then it creates shallow copies and replaces fields with previously copied values. * **Python Integration for Copy Operations**: Wired up Python's `__copy__`, `__deepcopy__`, and `__replace__` methods for FFI objects, ensuring proper copy semantics for both `@register_object` and `@c_class` types. Types without `enable_copy()` will now raise a `TypeError` when these methods are called. 🧠 **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 * **.pre-commit-config.yaml** * Updated the `ty check` pre-commit hook to include `--error-on-warning`, enforcing stricter type checking. * **CMakeLists.txt** * Added `src/ffi/extra/copy.cc` to the build system, incorporating the new C++ copy utilities. * **include/tvm/ffi/extra/copy.h** * Introduced the `DeepCopyValue` function declaration, providing a C++ API for deep copying `ffi::Any` values. * **include/tvm/ffi/reflection/registry.h** * Defined `refl::enable_copy` helper struct to facilitate registration of `__ffi_shallow_copy__` methods for C++ object types. * Extended `ObjectDef` to include a `def(enable_copy)` method, allowing types to easily opt-in to copy support. * **python/tvm_ffi/dataclasses/c_class.py** * Modified the `@c_class` decorator to call `_setup_copy_methods`, enabling Python copy methods for C++ classes. * **python/tvm_ffi/registry.py** * Implemented `_setup_copy_methods` to dynamically add `__copy__`, `__deepcopy__`, and `__replace__` to FFI object types based on the presence of `__ffi_shallow_copy__`. * Added helper functions (`_copy_supported`, `_deepcopy_supported`, `_replace_supported`, and their `_unsupported` counterparts) to handle the Python-side implementation and error reporting for copy operations. * Introduced `_get_deep_copy_func` and `_deep_copy_func_cache` for efficient lookup of the C++ deep copy function. * **src/ffi/extra/copy.cc** * Implemented the `ObjectDeepCopier` class, which performs a two-phase deep copy: graph traversal (DFS) to collect objects in topological order, followed by shallow copying and field replacement. * Provided the `DeepCopyValue` function, exposed as `ffi.DeepCopy` to the FFI, which leverages `ObjectDeepCopier`. * **src/ffi/testing/testing.cc** * Updated test object definitions (`TestIntPairObj`, `TestObjectBase`, `TestObjectDerived`, `SchemaAllTypesObj`) to use `refl::enable_copy()`, making them copyable for testing purposes. * **tests/python/test_copy.py** * Added a new test file with 17 tests covering various aspects of `copy.copy()`, `copy.deepcopy()`, and `__replace__` for FFI objects, including basic fields, new object identity, mutable fields, mutation isolation, derived type preservation, nested objects, shared references, and error handling for unsupported types. Activity * A new C++ implementation for deep copying FFI objects has been introduced. * Python's `copy` module functionality (`__copy__`, `__deepcopy__`, `__replace__`) has been integrated with FFI objects. * Comprehensive test cases have been added to validate the new copy mechanisms across various scenarios. * The build system and pre-commit hooks were updated to support the new C++ files and enforce stricter code quality. Using Gemini Code Assist The full guide for Gemini C
