Repository navigation
Revert "Fix non-deterministic constant folding output" - #32471
Xiaofei Han (xiaofeihan1) wants to merge 3 commits into
Conversation
|
Copilot resolve lint pipeline issue |
There was a problem hiding this comment.
🟢 Approval recommended
The only unresolved comment is a non-blocking documentation nit.
Pull request overview
Reverts a redundant constant-folding ownership check after the underlying initializer-lifetime issue was resolved.
Changes:
- Restores unconditional zero-copy tensor conversion.
- Restores the prior inline-data comment.
File summaries
| File | Review |
|---|---|
onnxruntime/core/optimizer/constant_folding.cc |
Reverts ownership-based deep-copy logic. Minor nit: the restored size-specific comment is inaccurate for string tensors. |
Review details
Suppressed comments (1)
onnxruntime/core/optimizer/constant_folding.cc:682
- This is also reached for string tensors, which
TensorToTensorProtostores inline regardless of size (tensorprotoutils.cc:2079-2097), so the size-specific explanation is inaccurate. Describe the storage state instead.
// The data is too small and has been inlined.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Fixed in 530e955: updated misspell to commit-pinned v1.28.0, whose prebuilt image avoids the expired Debian Bullseye metadata that broke Optional Lint. YAML validation, image smoke tests, secret scanning, and CodeQL passed. The new lint run needs maintainer approval before execution. |
|
Should we keep an assert in debug build? |
|
Xavier Dupré (@xadupre) Yes, I kept a Debug-build assertion to document and verify the invariant that constant-folding outputs own their buffers. I also updated the inline-data comment so it is accurate for both small and string tensors. Fixed in b90e91b. The focused ConstantFolding, aliased-output, and string-initializer tests pass. |
|
/run bots |
|
/run bots |
This reverts commit cd2d4a3.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Pass true directly with a parameter-name comment while preserving the Debug buffer-ownership assertion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 80e16013-a359-4acd-b6ca-583ffdd4f313
b90e91b to
7e7e3de
Compare
Reverts #29789
As discussed with the PR author in this comment, this PR is not necessary. We should revert it to avoid any misunderstandings regarding constant folding.