bugfix: add calculation of mm_embedding size in shm manager.#1298
bugfix: add calculation of mm_embedding size in shm manager.#1298shan-chen-feng wants to merge 2 commits intojd-opensource:mainfrom
Conversation
shan-chen-feng
requested review from
DongheJin,
JimHsiung,
RobbieLeung,
XuZhang99,
liutongxuan,
walsonyang and
yq33victor
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the serialization and size calculation for DiT forward outputs by replacing specialized functions with a generic get_vector_tensor_size helper and standard vector tensor utilities. It also updates the vector size header from uint64_t to int32_t. A review comment identifies that the new get_vector_tensor_size function should be placed in an anonymous namespace to comply with the repository's style guide regarding file-local functions.
| inline size_t get_dit_forward_output_size(const DiTForwardOutput& output) { | ||
| size_t size = type_size<uint64_t>; // vector size | ||
| for (const auto& tensor : output.tensors) { | ||
| inline size_t get_vector_tensor_size( |
There was a problem hiding this comment.
According to the repository style guide (Rule 177), file-local functions used only within a single .cpp file must be placed in an anonymous namespace to ensure internal linkage and avoid potential symbol collisions. Since get_vector_tensor_size is not declared in the header file, it should be moved into an anonymous namespace.
References
- File-local functions and variables (used only within a single .cpp file) must be placed in an anonymous namespace. (link)
shan-chen-feng
force-pushed
the
mm_embedding_bug_fix
branch
from
d63c6f5 to
ef37787
Compare
| } else { | ||
| sample_output.embeddings = embeddings; | ||
| output.sample_output = sample_output; | ||
| output.embedding = embeddings; |
There was a problem hiding this comment.
Can we remove the output.embedding = embeddings ？
shan-chen-feng
force-pushed
the
mm_embedding_bug_fix
branch
from
ef37787 to
a275173
Compare
2 participants
No description provided.