build(deps): Align protobuf and related dependencies with the gRPC v1.81.1 bump#516
Conversation
mc-nv
changed the title
mc-nv
changed the title
The protobuf bundled with gRPC v1.81.1 (v33.5) installs utf8_range as a separate CMake package and protobuf-config.cmake references the utf8_range::utf8_validity imported target. find_package(Protobuf CONFIG) with only Protobuf_DIR set fails at generate time with 'the target was not found'. Provide utf8_range_DIR alongside Protobuf_DIR.
Three breakages against the protobuf bundled with gRPC v1.81.1: - FieldDescriptor::name() now returns absl::string_view; wrap in std::string for concatenation. - JsonPrintOptions::always_print_primitive_fields was removed; use its designated replacement always_print_fields_with_no_presence. - MessageToJsonString's absl::Status return is nodiscard and fails the -Werror build when ignored; propagate it as an INTERNAL status.
6df36fb to
3df65f8
Compare
protobuf v33 makes DebugString() output intentionally unstable (injects a 'goo.gle/debugstr' marker that is not valid text format), which polluted the 'Server side auto-completed config' log and broke L0_logging's log_format parser. Serialize with TextFormat::PrintToString at the call site instead of LOG_PROTOBUF_VERBOSE.
Greptile SummaryThis PR repairs the nightly build broken by the gRPC v1.81.1 / protobuf v33 bump by propagating the new
Confidence Score: 4/5Safe to merge; all three protobuf v33 API adaptations are correct and the cmake fix is straightforward. The core changes — utf8_range_DIR propagation, absl::string_view conversion, option rename, and MessageToJsonString status check — are all correct and complete. The new verbose-logging block silently discards PrintToString's failure return and emits at kINFO rather than the VERBOSE level used everywhere else in the file; neither affects runtime correctness, only log quality. src/model_config_utils.cc — the new logging block around line 765 Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[GetNormalizedModelConfig] --> B[AutoCompleteBackendFields]
B --> C{LOG_VERBOSE_IS_ON 1}
C -- yes --> D[TextFormat::PrintToString config]
D --> E[LogMessage kINFO stream]
C -- no --> F[NormalizeModelConfig]
E --> F
G[ModelConfigToJson] --> H[JsonPrintOptions always_print_fields_with_no_presence]
H --> I[MessageToJsonString]
I --> J{status.ok?}
J -- no --> K[return INTERNAL error]
J -- yes --> L[fix up int64 fields in JSON]
M[CollectInt64Fields] --> N[field->name returns absl::string_view]
N --> O[std::string fullname = prefix + '::' + std::string field->name]
Reviews (1): Last reviewed commit: "fix: Log auto-completed model config wit..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| google::protobuf::TextFormat::PrintToString( | ||
| *config, &auto_completed_config); |
There was a problem hiding this comment.
TextFormat::PrintToString returns false when serialization fails (e.g., the message contains an unknown field that triggers strict printing). The return value is silently discarded here, so if it fails auto_completed_config will be empty and the log line will print nothing useful. Since this is only for diagnostics the cost is low, but a silent empty-string log can be confusing. Binding the result and logging a fallback is safer.
| google::protobuf::TextFormat::PrintToString( | |
| *config, &auto_completed_config); | |
| if (!google::protobuf::TextFormat::PrintToString( | |
| *config, &auto_completed_config)) { | |
| auto_completed_config = "<serialization failed>"; | |
| } |
| triton::common::LogMessage( | ||
| __FILE__, __LINE__, triton::common::Logger::Level::kINFO, | ||
| "Server side auto-completed config: ", false) | ||
| .stream() | ||
| << auto_completed_config; |
There was a problem hiding this comment.
LOG_VERBOSE_IS_ON(1) but LogMessage is called with Logger::Level::kINFO. This means when verbose level 1 is enabled the auto-completed config is emitted as an INFO message, not a VERBOSE message, which could clutter INFO-level log streams. The rest of the file uses LOG_VERBOSE(1) << ... for the same pattern; using it here would match codebase conventions and emit at the correct level.
| triton::common::LogMessage( | |
| __FILE__, __LINE__, triton::common::Logger::Level::kINFO, | |
| "Server side auto-completed config: ", false) | |
| .stream() | |
| << auto_completed_config; | |
| LOG_VERBOSE(1) << "Server side auto-completed config: " | |
| << auto_completed_config; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
3 participants
What does the PR do?
Fixes the nightly core build against protobuf v33 (gRPC v1.81.1 third_party bump):
utf8_range_DIRinto the triton-core ExternalProject cache args — protobuf v33's CMake package config requires theutf8_rangepackage config to resolve.model_config_utils.ccto the protobuf v33 API:FieldDescriptor::name()now returnsabsl::string_view(explicitstd::stringconversion),always_print_primitive_fieldsis renamed toalways_print_fields_with_no_presence, and the now-nodiscardMessageToJsonString()status is checked and surfaced as an INTERNAL error.Checklist
<commit_type>: <Title>Related PRs:
Where should the reviewer start?
CMakeLists.txt—utf8_range_DIRpropagationsrc/model_config_utils.cc— protobuf v33 API adaptationsTest plan:
Nightly build pipeline on internal GitLab CI.
Caveats:
None.
Background
The gRPC v1.81.1 / protobuf v33 update in triton-inference-server/third_party#76 broke the nightly build across the Triton repos; this PR chain repairs it.
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
- Resolves: TRI-1608
CI (internal): [#58489960](http://tritonserver.local/ci/pipelines/58489960)