[build] include timestamp in stamped versions#145
Conversation
There was a problem hiding this comment.
👋 Review Summary
Clean, focused PR — the approach of adding a BUILD_TIMESTAMP volatile variable to disambiguate same-day builds is straightforward and correctly applied across all wheel targets, the version generator, and documentation.
🛡️ Key Risks & Issues
No blocking issues found. The implementation is consistent and PEP 440 compliant (0.0.1+20260409.113826.2cd6c5a9 — three valid alphanumeric segments separated by dots in the local part).
One asymmetry worth being aware of for future maintenance:
- Fallback behavior differs between codepaths:
gen_version_info.pygracefully degrades toBUILD_DATEwhenBUILD_TIMESTAMPis absent (status_vars.get("BUILD_TIMESTAMP", build_date)). However, thepy_wheelBUILD files reference{BUILD_TIMESTAMP}directly in the version template — if the variable is ever missing fromworkspace_status.sh, the placeholder would remain as a literal string, producing an invalid PEP 440 version. This isn't a problem today sinceBUILD_TIMESTAMPis a puredatecall with no external dependency, but a comment inworkspace_status.shnoting that py_wheel templates depend on this name would help future maintainers.
🧪 Verification Advice
- The PR's validation uses
bazel build --nobuild, which only exercises the analysis phase (loading + configuration). Stamp substitution happens at execution time, so--nobuildcannot catch placeholder resolution issues. Consider running at least one wheel target without--nobuildto confirm the final version string renders correctly (e.g., inspect the generated_version_info.pyor the wheel filename under.dist). - There are currently no unit tests for
gen_version_info.py. This is a pre-existing gap, but as the version format grows more complex, a lightweight test asserting the output format against a synthetic status file would provide a safety net against regressions.
💡 Thoughts & Suggestions
- Good choice keeping
BUILD_TIMESTAMPvolatile (noSTABLE_prefix) — this means incremental builds won't be invalidated just because the clock moved, preserving cache efficiency. - The
.separator in the timestamp format (%Y%m%d.%H%M%S) plays well with PEP 440's local segment delimiter, resulting in a naturally readable version:version+date.time.commit. - Minor nice-to-have: the missing-newline-at-EOF fixes in the BUILD files are a welcome cleanup.
🤖 Generated by Qoder • View workflow run
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dc76d2b83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3dc76d2 to
d1a32d3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1a32d3f91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
d1a32d3 to
e1931ff
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1931ffe33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e1931ff to
32df8ab
Compare
32df8ab to
132dec9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 132dec93fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| echo "STABLE_KVCM_VERSION 0.0.1" | ||
| # Used by py_wheel version templates; keep stable so repeat packaging for the | ||
| # same commit invalidates the cached wheel action and gets a fresh version. | ||
| echo "STABLE_BUILD_TIMESTAMP $(date +%Y%m%d.%H%M%S)" |
There was a problem hiding this comment.
Avoid PEP 440 renormalizing stamped times
For builds before 10:00, this dot makes HHMMSS a separate numeric local-version segment in every py_wheel template. I checked the pinned rules_python wheelmaker path from open_source/deps/git.bzl: after stamp expansion it normalizes with packaging.version.Version, so 20260619.023500 becomes 20260619.23500 in the wheel filename/METADATA, while the generated _version_info.py/C++ FULL_VERSION still report 20260619.023500. Those connector logs no longer identify the installed wheel version exactly for those builds; use a single numeric segment like %Y%m%d%H%M%S or prefix the time segment with a letter.
Useful? React with 👍 / 👎.
Summary
STABLE_BUILD_TIMESTAMPinYYYYMMDD.HHMMSSformat and use it in all stamped KVCM Python wheel versions to avoid same-commit package version collisions.build_version.hplus a stampedbuild_version.cc, so wall-clock changes rebuild the small version object and relink binaries without forcing downstream source recompilation through a changing header.kPascalCaseconstant style (kKvcm*).extern const char ...[] = ...) to match the header declarations and avoid undefined references from consumers.Validation
PYTHON_BIN_PATH=/usr/bin/python3 bazel build //kv_cache_manager/common:build_versionPYTHON_BIN_PATH=/usr/bin/python3 bazel build //kv_cache_manager/service:service //kv_cache_manager/common:crash_handlerwith the polluted shared disk cache disabled; full link completed successfully.PYTHON_BIN_PATH=/usr/bin/python3 bazel build --nobuild //kv_cache_manager/common/test:CrashHandlerTest //kv_cache_manager/common:build_versionPYTHON_BIN_PATH=/usr/bin/python3 bazel build --nobuild //kv_cache_manager:kv_cache_manager_bin //kv_cache_manager:main //kv_cache_manager/common/test:CrashHandlerTest //kv_cache_manager/service/test:CommandLineTest //kv_cache_manager/client/pybind:kvcm_py_client_lib_wheel //kv_cache_manager/py_connector/sglang:kvcm_sglang_connector_wheel //kv_cache_manager/py_connector/vllm:kvcm_vllm_connector_wheel //kv_cache_manager/optimizer/pybind:kvcm_py_optimizer_lib_wheelgit diff --check origin/main..HEADbazel/gen_version_info.pygeneration for stamped C++ header/source status inputsnm -Con the generated object and Bazellibbuild_version.areportskv_cache_manager::kKvcm*symbols as globalR, not localr.build_version.hlinks against Bazellibbuild_version.aand prints the generated full version.rg -n '\bKVCM_(VERSION|GIT_COMMIT|GIT_COMMIT_FULL|GIT_REPO|BUILD_DATE|BUILD_TIMESTAMP|BUILD_TIME|FULL_VERSION)\b|kKVCM' .returns no matches