fix(tf): find external protobuf for TensorFlow 2.21 - #5986
Conversation
Backport conda-forge/deepmd-kit-feedstock patch 0001 at d02138263b0b30629fe28c68c8bcecc2b8644513. Agent: ChatGPT Model: GPT-5.6 Sol
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe TensorFlow CMake module now derives and validates a Protobuf prefix from the runtime library, supports flat and multiarch layouts, restores the original ChangesTensorFlow discovery
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR improves external Protobuf discovery while preserving the TensorFlow runtime library and caller settings; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5986 +/- ##
==========================================
- Coverage 79.10% 77.44% -1.66%
==========================================
Files 1105 1105
Lines 130981 130975 -6
Branches 4771 4765 -6
==========================================
- Hits 103609 101435 -2174
- Misses 25686 27893 +2207
+ Partials 1686 1647 -39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Non-blocking. The fix is sound for the environment it came from, and I confirmed the backport is byte-identical to the feedstock patch. Two notes and one fact about coverage.
Worth stating plainly: no CI job executes these lines. The branch is only entered when google/protobuf/type.pb.h is absent from the TensorFlow tree, and CI installs TensorFlow from pip wheels, which bundle that header (checked on a local pip TensorFlow: .../tensorflow/include/google/protobuf/type.pb.h is present, and it is exactly the sentinel find_path looks for at line 224). So the green checks on this PR say nothing about the change, and "Full build is left to upstream CI" does not hold for this path, because this repository's CI does not cover it. That is not a reason to block -- there is no reasonable unit test for a find-module branch like this, and adding a conda-forge-TensorFlow build job is far more than this PR should carry -- but it is worth being explicit that the change ships unexercised.
Keep an ldd-discovered protobuf library pinned while using its installation prefix to guide FindProtobuf. Fall back to the normal external package search when TensorFlow exposes no protobuf runtime dependency. Coding-Agent: Codex Codex-Version: codex-cli 0.149.0 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
The review summary coverage note is addressed as well: the PR description now explicitly states that repository CI does not execute the external-header branch and removes the previous claim that a full build was left to upstream CI. It documents the focused CMake configurations used to validate both the pinned runtime-library path and the TensorFlow 2.21 fallback path. The inline threads have been addressed and resolved, and reviews have been re-requested. Coding agent: Codex |
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Both earlier points are addressed, verified against the code rather than the replies.
The pin survives: Protobuf_LIBRARY is no longer unset, and _protobuf_find_libraries in CMake's FindProtobuf (lines 511-517) takes the elseif(${name}_LIBRARY) branch and skips find_library, so the exact runtime-linked library is kept. Because Protobuf_INCLUDE_DIR is no longer pre-set, find_path now genuinely runs and Protobuf_ROOT can steer it, and the plain fallback happens only when the scan came up empty -- the TensorFlow 2.21 case. The protobuf-lite sentence is gone and the comment now describes only what is demonstrated. That is the shape I was asking for.
One gap in it, inline -- and it is a gap in what I suggested, not something you introduced on your own.
Two things worth flagging for your judgement rather than as defects:
The first revision was byte-identical to the feedstock patch, which meant it was already running in conda-forge's production builds. This revision is new code that has not run anywhere, and CI still does not execute it: the branch is entered only when google/protobuf/type.pb.h is missing from the TensorFlow tree, while CI installs TensorFlow from pip wheels, which ship that header (checked on a local pip TensorFlow -- and it is exactly the sentinel find_path looks for at line 224). So the validation basis is weaker than it was for the first revision, not stronger. That is an argument for keeping the new logic as small and as obviously-correct as possible.
Second, now that this diverges from 0001-find-external-protobuf-for-tensorflow-2.21.patch, the feedstock patch will need refreshing once this lands, or it will conflict.
Separately, I looked at whether the fallback should avoid linking a system protobuf at all when the scan finds nothing, and concluded it should not: deepmd calls protobuf runtime symbols directly (ParseFromString at source/api_cc/src/DeepPotTF.cc:439, TextFormat::Parse at source/api_cc/src/common.cc:1274-1277), so something has to be linked. Recording that here so it does not get raised again.
Verify the inferred protobuf prefix contains the headers before using it, and handle multiarch library directories by checking one additional parent. Leave FindProtobuf on its normal search path when neither layout matches. Coding-Agent: Codex Codex-Version: codex-cli 0.151.0 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
The latest review comment is addressed in 3499e8a. The inferred Protobuf prefix is now accepted only when it contains I validated the flat runtime-linked layout, the multiarch runtime-linked layout, and the TensorFlow 2.21 fallback with no recorded Protobuf runtime dependency. The expected library and headers were selected in all three configurations, and caller-provided The inline response is here: #5986 (comment). Review has been re-requested from @wanghan-iapcm. Coding agent: Codex |
Summary
Adapt the conda-forge TensorFlow 2.21 external-Protobuf fix for upstream use.
Validation
<prefix>/liblayout even when a different protobuf prefix was globally searchable.<prefix>/lib/<triplet>to headers in<prefix>/includeand restored the caller'sProtobuf_ROOT.cmake-formatcheck passed forsource/cmake/Findtensorflow.cmake.ruff check .passed.ruff format .left 1913 files unchanged.Repository CI does not exercise the external-header branch because its pip TensorFlow packages bundle
google/protobuf/type.pb.h. The focused CMake configurations above validate the changed lookup behavior directly.Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit