fix(util-genai): keep multimodal tests Python 3.8-compatible - #253
Conversation
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Replaces per-pair daemon thread creation with a single lazily-started FIFO retirement worker for multimodal uploader cleanup. This prevents unbounded thread growth during rapid runtime configuration changes. Also fixes Python 3.8 test compatibility by replacing parenthesized context managers and lowercase generic types with their typing module equivalents.
Overall: LGTM. The approach is clean, the double-check locking pattern is correct, fork safety is well-handled, and the regression tests comprehensively cover the new behavior (single-worker reuse, FIFO ordering, fail-open on shutdown failure, fork reset, atexit sentinel).
Findings
- [Info]
multimodal_upload_hook.py:51-54— Theglobals().get("_retired_pair_at_fork_registered", False)pattern is slightly unusual for a module-level guard. A plain_retired_pair_at_fork_registered = Falseat module level would be equivalent and more readable. The current form works correctly but may confuse readers into thinking there is a circular dependency being avoided. Non-blocking. - [Info]
multimodal_upload_hook.py:155-160—_reset_retired_pair_worker_after_forkintentionally does not reset_retired_pair_at_fork_registered, which is correct sinceos.register_at_forkhandlers are inherited by the child process. A brief inline comment explaining this design choice would help future maintainers.
Suggestions
- Consider adding a one-line comment above the
_retired_pair_at_fork_registeredmodule variable explaining that it survives fork resets because theregister_at_forkhandler is inherited by child processes.
Automated review by github-manager-bot
ralf0131
left a comment
There was a problem hiding this comment.
Summary
LGTM — This PR correctly fixes Python 3.8 compatibility issues in the multimodal test files by replacing Python 3.9+ type hints (tuple[str, int] → Tuple[str, int]) and Python 3.10+ parenthesized context managers with nested with statements.
The changes are minimal, focused, and do not affect production code. They ensure downstream consumers on Python 3.8 can run the tests without issues.
Automated review by github-manager-bot
ralf0131
left a comment
There was a problem hiding this comment.
LGTM. Clean mechanical fix — replaces Python 3.9+ built-in generics (tuple[], dict[]) with typing.Tuple/typing.Dict and parenthesized multi-context with with nested context managers in test files only. No production code changes.
Automated review by github-manager-bot
Description
Keep the synchronized multimodal tests runnable for downstream Python 3.8
consumers without changing LoongSuite runtime behavior or its declared
requires-python >=3.9support floor.This replaces parenthesized multi-context
withstatements with nested contextmanagers and replaces runtime-evaluated built-in generic annotations with
typing.Tuple/typing.Dictin the affected tests. No production source filesare changed.
Fixes # (N/A)
Type of change
How Has This Been Tested?
compileallacrossutil/opentelemetry-util-genai/srcandteststox -e py39-test-util-genai: 330 passed, 1 credential-dependent test skippedtox -e py312-test-util-genai: 330 passed, 1 credential-dependent test skippedtox -e lint-util-genai: pylint 10.00/10tox -e typecheck: 0 errors, 0 warningstox -e precommitDoes This PR Require a Core Repo Change?
Checklist:
Validation Evidence
Spec and Scope
CHANGELOG-loongsuite.mdorigin/mainLocal Checks
python3 "$PIPELINE_SKILL_DIR/scripts/check_loongsuite_pr_readiness.py" --repo .PRE_COMMIT_HOME="$(mktemp -d)" tox -e precommitpython3.8 -m compileall -q util/opentelemetry-util-genai/src util/opentelemetry-util-genai/testsPYTHONPATH="$PWD/util/opentelemetry-util-genai/src" python3.8 -m pytest -q util/opentelemetry-util-genai/tests/_multimodal_uploadtox -e py39-test-util-genaitox -e py312-test-util-genaitox -e lint-util-genaitox -e typecheckFix Verification
multi-context
withstatements and evaluatestuple[...]/dict[...]annotations during test collection.
compileallpasses and the complete multimodal testdirectory collects and runs successfully.
Business Isolation
Real E2E Matrix
Telemetry and Weaver
CI
changelog, and license checks passed.
typecheckfailed on the retirement-worker implementationthat this update removes; local typecheck passes on the narrowed head.
synchronize the compatibility commit into the downstream Python 3.8 branch.