Repository navigation
refactor(container): share the PyNvVideoCodec install guard - #15595
dmitry-tokarev-nv merged 4 commits into
Conversation
One script reads its deny list from codec_policy.yaml. Signed-off-by: Eric Curtin <eric.curtin@docker.com>
|
👋 Hi ericcurtin! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe pull request adds a shared PyNvVideoCodec install guard that reads the codec policy. Three runtime templates invoke it with pin ChangesPyNvVideoCodec install validation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change moves the PyNvVideoCodec install checks into one shared guard used by the three runtime templates. No concrete problems were found, and the existing failure cases remain covered by tests. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 39.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 4 files. (3 skipped: 3 unsupported.)
Comment |
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
|
@dmitry-tokarev-nv @dagil-nvidia PTAL when you get a chance. Thank you! |
There was a problem hiding this comment.
Not approving yet. The P2 below makes the shared guard weaker than the two heredocs it replaces, so it needs a fix first. Read at 6e99032266.
[P2] Build-suffixed codec libraries pass the new guard. Inline at check_pynvvideocodec.py:75, confirmed end to end on the CI images in that thread.
[P3] Restore the empty-package test case. Reply on the existing thread at test_check_pynvvideocodec.py:128.
CI on this head: the guard step ran on amd64 and arm64 in the vLLM, SGLang and TensorRT-LLM image builds, and all seven required checks pass.
What I verified, with controls.
- The new guard exits 0 on the real 2.2.3 install in the vLLM, SGLang and TensorRT-LLM CI images from #14925 (
06c2ff404e) and from this head. With--pinned 2.2.2it exits 1 in each. The old heredoc exits 0 on the same installs. - In 48 BuildKit builds on this head's images, the cases in the description each fail the new guard: second distribution, second source tarball, denied library, and empty package. A wrong version fails too. The clean install passes.
- Each template's interpreter imports
yamland the guard in the current base images: vLLMv0.30.0-ubuntu2404, SGLangv0.5.19-cu130-runtime, and TensorRT-LLM1.3.0rc29. test_pynvvideocodec_spec.pypasses 14 tests. A wrong pin or a removed guard call in the vLLM template fails 2 of them each.- Rendering the three CUDA runtime Dockerfiles changes only the guard sections.
test_check_pynvvideocodec.py(22) andtest_scan_codecs.py(23) pass locally and in the CI unit lane.- This head's runtime-test images pass the dependency, multimodal and NVDEC test files: 872 tests, 0 failures, with the NVDEC GPU decode tests run in each image.
|
/ok to test 6e99032 |
There was a problem hiding this comment.
Previously reported defects still present:
- Original discussion: Still present at the reviewed head:
check_pynvvideocodec.pydelegates denied-library detection topolicy.violates(...), so build-suffixed denied codec libraries such aslibavcodec_nvc.so.*remain outside the deny globs described in the existing thread. - Original discussion: Still present at the reviewed head:
test_a_partial_package_failsparameterizes only the one-library cases and does not restore the empty package case discussed in the thread. - Original discussion: The shared guard still only calls
policy.violates(...), so codec-family libraries with build suffixes such aslibavcodec_nvc.so.63do not match the policy's plain/hash/dynamo deny globs and pass where the replaced prefix-based guards failed. Restore family-prefix detection before applying policy waivers. - Original discussion: Verified still present at the reviewed head:
check_pynvvideocodec.pynow relies onpolicy.violates(...), whose deny globs cover plain, hash-renamed, and_dynamocodec library names but not other build-suffixed names such aslibavcodec_nvc.so.63,libswscale_nvc.so.10, orlibx264_nvc.so.164. The vLLM and SGLang runtime guard can therefore accept PyNvVideoCodec installs that the replaced prefix-based heredocs rejected. Restore family-prefix rejection before applying the policy waivers, and add a suffixed-library regression case. - Original discussion: Verified still present: test_a_partial_package_fails still lacks the empty-package parameter, so a guard that skips required-library validation when no bundled libraries are found would not be caught by this test.
- Original discussion: Verified still present: the guard still delegates denial to policy globs that do not match build-suffixed third-party names such as libavcodec_nvc.so.63, libswscale_nvc.so.10, or libx264_nvc.so.164, so those bundled codec libraries can pass the shared guard.
- Original discussion: Verified still present: the shared guard relies on codec_policy.yaml deny globs via policy.violates(), and those globs reject plain, hash-renamed, and _dynamo library names but do not reject other build-suffixed denied families such as libavcodec_nvc.so.63, libswscale_nvc.so.10, or libx264_nvc.so.164. The removed vLLM/SGLang heredocs used basename prefix matching for denied families, so those suffixed libraries would have failed before and now pass the guard.
✅ Dynamo PR CI passed — run 37480550987 (attempt 1) on
|
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Tested suggestions for the two open items, at 6e99032266. Please apply all three with "Add suggestion to batch", so that the fix and its tests land in one commit. When they land, I will re-run my probes on the new head and approve.
[P2] The fix is a suggestion on the check_pynvvideocodec.py:75 thread, and its test is at test_check_pynvvideocodec.py:120.
[P3] The suggestion is at test_check_pynvvideocodec.py:128.
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
|
@dmitry-tokarev-nv Applied all three suggestions in 1d27fb1. PTAL when you get a chance. Thank you! |
|
/ok to test 1d27fb1 |
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
I approve 1d27fb141e. My P2 and P3 are fixed, and I resolved my three threads with the evidence. The fix and its tests are my suggestions, applied as written, so this approval partly reviews my own code.
[P3] No test protects the policy.violates(...) branch at check_pynvvideocodec.py:86. Without the branch, all 25 tests still pass. This gap is in the fix that I suggested. No push is needed for this approval, and a follow-up PR is fine.
Open: the two bot threads, at test line 130 and on the spec-test docstring, are yours to resolve, because main requires resolved threads.
Not run in CI: the SGLang image build on the v0.5.21 base with this guard. I tested that base in an image built from main.
What I measured at 1d27fb141e, with controls.
- The guard file and its test file at this head are byte-identical to the files at
6e99032266with my three suggestion blocks applied. The only other change removes a test docstring, after a bot comment. - On fake installs, a build-suffixed library from each of the nine old
DENIEDfamilies now fails, as with the old heredoc. The table is on thecheck_pynvvideocodec.py:75thread. - In the vLLM, SGLang and TensorRT-LLM runtime-test images for
6e99032266, the guard exits 0 on the real 2.2.3 install. It exits 1 with--pinned 2.2.2, and 1 with a planted suffixed library. - The 62 tests in the three test files of this PR pass in all three images.
- Mutants: against a guard that skips the required-library check on an empty package, only the restored
()case fails. Against a guard without the fix, only the two new suffixed cases fail. - A merge with
mainat7d82c10d9ea050de5147d94894b99d664d60d37cis clean ingit merge-tree. The 62 tests pass on the merged tree in the vLLM image, and in the SGLang image formainatc746b5de1b.mainthen moved toc5d615ea59193aa7805da7979ce22567289fa26a. The merge with it is clean too, and its two new commits change only files undercomponents/src/dynamo/. - Behavior change: a suffixed
libavformatorlibavutilnow fails too. The waivers cover onlylibavformat.so*andlibavutil.so*directly in the package folder, and the real wheel bundles only those names. - CI on this head: PR run 37480550987 succeeded. The guard step ran on amd64 and arm64 in the vLLM, SGLang and TensorRT-LLM image builds. The
dynamo-runtime / test / sequential cuda13.0, amd64job passed all 25 guard tests. PR-XPU 37480550144 succeeded with its XPU lanes skipped. Pre Merge 37398246272 succeeded, and its PyNvVideoCodec spec hook passed.
The SGLang v0.5.21 base, which no CI run used with this guard.
mainmoved the SGLang base from v0.5.19 to v0.5.21 in5c8b22b3d9, after the merge base of this branch. The guard importsyamlthroughscan_codecs.py, and the old heredoc did not.- Before the guard step, the template installs pip packages only with
--no-deps, apart fromgpu_memory_service(which needsmsgspecanduvloop) andgoogle-crc32c. Soyamlmust come from the base image. - In the SGLang runtime-test image for
mainatc746b5de1b,/opt/sglang/bin/python3imports PyYAML 6.0.1. The PyYAML dist-info folder is dated 2026-10-01, likesglang-0.5.21from the base. PyNvVideoCodec, which the template installs, is dated 2026-10-06. The guard step uses this interpreter: in the CI build of this head, the guard found the install under/opt/sglang. - In that image, the guard from the merged tree exits 0 on the real install, and 1 with a planted
libavcodec_nvc.so.63.
The P3, with a tested change to one test.
At 6e99032266, policy.violates(...) was the only way to deny a library, so every denied-library test needed it. The fix adds a prefix match, which also catches every current test case. So a guard without the policy.violates(...) branch still passes all 25 tests.
The branch still matters. It is the only path for a name that matches a wildcard deny glob but no derived family. Without the branch, libavfoo_dynamo.so.1 and libswfoo_dynamo.so.2 pass the guard, and a new glob such as **/libfoo*.so* does not reach it.
This change to the first lines of the test at test_check_pynvvideocodec.py:143 covers the branch. The rest of the test stays the same.
@pytest.mark.parametrize("glob", ["**/libfoo.so*", "**/libfoo*.so*"])
def test_a_family_added_to_the_policy_reaches_the_guard(tmp_path, glob):
site = _install(tmp_path, libs=(*GOOD_LIBS, "libfoo.so.1"))
_check(site)
doc = yaml.safe_load(DEFAULT_POLICY.read_text(encoding="utf-8"))
doc["deny_globs"].append(glob)With this change, the file passes 26 of 26 at this head. Without the branch, only test_a_family_added_to_the_policy_reaches_the_guard[**/libfoo*.so*] fails. black 23.1.0, isort 5.12.0, flake8 7.3.0 and ruff 0.5.2 pass on the changed file.
Overview:
Share the PyNvVideoCodec install guard across the vllm, sglang and trtllm images.
Details:
codec_policy.yaml.RUNsteps pass in a container build on the real 2.2.3 wheel, and each negative control (second dist, second tarball, denied lib, empty package) still fails. No full image build.Where should the reviewer start?
container/compliance/check_pynvvideocodec.pyRelated Issues
🔗 This PR is linked to an issue:
Summary by CodeRabbit