feat: add multimodal runtime config hot-reload#249
Conversation
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Introduces a process-wide MultimodalRuntimeConfig snapshot with generation-aware uploader hot-reload. The design is clean: frozen MultimodalConfigSnapshot for immutable reads, RLock-guarded MultimodalRuntimeConfig for thread-safe updates, and generation counters to detect stale uploader pairs. 116 tests with good coverage of edge cases.
Findings
- [Warning]
config.py:287— SLS endpoint/auth/credential fields are copied into the merged snapshot but never updatable viaupdate_multimodal_runtime_config(). Silent no-op if a caller passes these keys. - [Info]
multimodal_upload_hook.py:78—except TypeErrorfallback for legacy hooks could mask legitimate TypeErrors from inside hook factories. - [Info]
multimodal_upload_hook.py:239— Permanently-failed generation blocks all future uploads for that config version with no visible log.
Suggestions
- Add a short docstring or comment on
_normalize_and_validateexplaining which fields are intentionally read-only (security: no runtime credential rotation). - Consider
inspect.signatureinstead ofexcept TypeErrorfor the legacy hook fallback path. - A
_logger.warningat the_failed_generationassignment would help operators diagnose silent upload outages.
Cross-repo Note
No impact on loongsuite-pilot — the multimodal upload subsystem lives entirely within the Python agent util package.
Automated review by github-manager-bot
| uploader_hook_name=merged["uploader_hook_name"], | ||
| pre_uploader_hook_name=merged["pre_uploader_hook_name"], | ||
| sls_project=merged["sls_project"], | ||
| sls_logstore=merged["sls_logstore"], |
There was a problem hiding this comment.
[Warning] _normalize_and_validate copies sls_endpoint, sls_auth_type, sls_access_key_id, sls_access_key_secret, and sls_sts_token from old into merged, but the elif chain (lines 261-285) never handles these keys. Passing update_multimodal_runtime_config(sls_endpoint=...) would silently be a no-op. If this is intentional (e.g. for security, to prevent credential rotation via the runtime API), consider adding a comment or logging a warning for unsupported keys. Otherwise, add elif branches for these fields.
| ) -> Optional[object]: | ||
| try: | ||
| return hook(snapshot) | ||
| except TypeError: |
There was a problem hiding this comment.
[Info] except TypeError is used to support legacy zero-arg hooks. If a hook factory raises TypeError internally for a legitimate reason (e.g. bad argument type from caller), it would be silently caught and retried with no args. Consider using inspect.signature to check the parameter count instead, which is more robust.
| generation = cfg.uploader_generation | ||
|
|
||
| with _uploader_pair_lock: | ||
| if _failed_generation == generation: |
There was a problem hiding this comment.
[Info] When _failed_generation == generation, get_or_rebuild_uploader_pair permanently returns (None, None) for that generation. Consider adding a log line when setting _failed_generation (line 261) to aid debugging when uploads silently stop after a hook build failure.
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Introduces a process-wide MultimodalRuntimeConfig snapshot with generation-aware uploader hot-reload. Clean design: frozen MultimodalConfigSnapshot for immutable reads, RLock-guarded MultimodalRuntimeConfig for thread-safe updates, and generation counters to detect stale uploader pairs. 116 tests with good coverage of edge cases.
The findings from the initial review are all non-blocking improvement suggestions (see inline comments). The SLS credential fields being read-only is intentional (security: no runtime credential rotation). No correctness, performance, or compatibility blockers found. Approving to unblock CI workflows for this first-time contributor.
Improvement Suggestions (non-blocking)
- Consider
inspect.signatureinstead ofexcept TypeErrorfor the legacy hook fallback path. - A
_logger.warningat the_failed_generationassignment would help operators diagnose silent upload outages.
Positive Notes
- Solid test coverage (116 tests) covering edge cases.
- Clean separation between frozen snapshot (reads) and RLock-guarded config (writes).
- Generation-counter pattern is a robust way to handle concurrent hot-reload.
Automated review by github-manager-bot
ralf0131
left a comment
There was a problem hiding this comment.
Review Summary
Overall Assessment
This PR introduces a well-designed runtime configuration system for multimodal upload with generation-aware hot-reload. The implementation is clean, thread-safe, and backward-compatible. Approved.
Severity Breakdown
- 🔴 Critical: 0 🟠 High: 0 🟡 Medium: 0 🔵 Low: 0 ℹ️ Info: 3 ✅ Praise: 5
Key Findings
No blocking issues found. The code is production-ready.
Positive Notes
- Thread-safe config management:
MultimodalRuntimeConfigusesthreading.RLockwith a double-checked locking pattern inget_or_rebuild_uploader_pair()— generation tracking prevents unnecessary rebuilds and concurrent build races are handled correctly. - Backward compatibility: Legacy zero-arg hooks are supported via
_call_hookfallback, andget_or_load_uploader_pair(the old lazyOnce()path) is preserved alongside the new generation-aware path. - Graceful resource cleanup: Retired uploader pairs are shut down in background daemon threads via
_schedule_retired_pair_shutdown, preventing resource leaks during config hot-reload. - Comprehensive test coverage: 116 tests including concurrency scenarios (
test_concurrent_get_or_rebuild), stale-build discard (test_stale_build_discarded), and strategy version bump validation. The autousereset_runtime_statefixture inconftest.pyensures test isolation. - Documentation: README and CHANGELOG updated with clear API usage examples and migration notes.
Info
_call_hookusesexcept TypeErrorto fall back from snapshot-based to legacy zero-arg hooks. While pragmatic for backward compat, a realTypeErrorfrom inside hook logic (not signature mismatch) would be caught silently on the first attempt. The second zero-arg attempt would then also raise, so this is somewhat self-correcting — but worth being aware of if debugging hook failures._schedule_retired_pair_shutdownspawns a daemon thread per retired pair. Under rapid config churn, multiple daemon threads could accumulate briefly. Each shuts down quickly, so this is a minor concern, not a blocker.- Python 3.8 compatibility is maintained (avoiding combined
with (...)context managers in tests), which aligns with the reporuntime.python: "3.9"target.
Automated review by github-manager-bot
|
Thanks for your contribution! The code quality looks good — I've completed my code review and posted inline feedback. However, CI is currently failing on three checks:
Could you please fix these CI failures so the PR can be merged? You can find the failure details in the Actions tab. Common causes:
Once CI is green, the PR will be ready for merge. Happy to help if you get stuck on any of the failures! |
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Introduces a process-wide MultimodalRuntimeConfig snapshot with generation-aware uploader hot-reload. The contributor has addressed all previous review findings in commit 50c52a49 — excellent response to feedback. The implementation is clean, thread-safe, and backward-compatible. Approved.
Severity Breakdown
🔴 Critical: 0 | 🟠 High: 0 | 🟡 Medium: 0 | 🔵 Low: 0 | ℹ️ Info: 2 | ✅ Praise: 4
Changes Since Last Review
- [Addressed] SLS credential/endpoint fields are now explicitly read-only via
_READ_ONLY_RUNTIME_FIELDSwith a warning log — credentials can only come from environment variables. Good security practice. - [Addressed]
_call_hooknow properly wrapsinspect.signaturewithexcept (TypeError, ValueError)— handles C-extension/builtin hooks gracefully. - [Addressed]
get_or_rebuild_uploader_pairrefactored into_resolve_cached_uploader_pair+_commit_uploader_pair— much cleaner separation of the cache-check and commit phases. - [Addressed] Added
_logger.warningfor failed generation builds — improves observability.
Info (Non-blocking)
- [Info]
config.py—_VALID_HOOKSrestricts hook names to{"arms", "fs"}. Custom third-party hooks registered via entry points are silently rejected and fall back to"fs". This is likely intentional for the LoongSuite distribution, but consider documenting this restriction in the README or docstring. - [Info]
config.py—MultimodalRuntimeConfig.get_snapshot()readsself._snapshotwithout holding_lock. This is safe under CPython's GIL (reference assignment is atomic, snapshot is frozen), but worth noting if the project ever targets free-threaded Python (3.13+--disable-gil).
Positive Notes
- Praise — Excellent response to review feedback. All findings from the initial review were addressed precisely and thoroughly.
- Praise — The double-checked locking pattern with
_building_generationis correct: concurrent callers for the same generation get(None, None)instead of blocking, stale builds are discarded on commit, and failed generations are cached to avoid retry storms. - Praise — Clean separation of
strategy_version(affects pre-uploader behavior) vsuploader_generation(triggers uploader rebuild) — well thought-out. - Praise — Comprehensive test coverage (116 passed, 1 skipped) including generation bumps, hook validation, and stale build detection.
Automated review by github-manager-bot
Description
Add process-wide multimodal runtime configuration so upload strategy and
uploader/pre-uploader hooks can be updated without restarting the process.
Previously, multimodal upload settings were read mainly at bootstrap / first
use (
os.getenv+ lazyOnceuploader load), so changing configuration afterstartup did not reliably rebuild uploaders. This change introduces:
MultimodalRuntimeConfig/MultimodalConfigSnapshotwithstrategy_versionanduploader_generationget_or_rebuild_uploader_pair()for hot rebuildupdate_multimodal_runtime_config(**fields)Bootstrap still comes from the existing
OTEL_INSTRUMENTATION_GENAI_MULTIMODAL_*environment variables. Callers can then update the in-process snapshot at
runtime through the public API.
No new runtime dependencies.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Unit tests under
util/opentelemetry-util-genai/tests/_multimodal_uploadResult: 116 passed, 1 skipped (OSS integration skipped without credentials)
Key coverage added/updated:
test_multimodal_runtime_config.py(snapshot update, generation bumps, hook validation)test_multimodal_upload_hook.py(generation-aware rebuild)Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.