Revert "Remove nvenc/dec for xenna 0.1.6"#1374
Conversation
Greptile SummaryThis PR reverts changes from #1202 to address GPU allocation issues observed with Xenna's GPU usage vs Ray-allocated resources. Key changes:
Impact:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ClipTranscodingStage
participant cosmos_xenna
participant Resources
participant XennaAdapter
participant XennaResources
User->>ClipTranscodingStage: __post_init__(encoder="h264_nvenc")
ClipTranscodingStage->>cosmos_xenna: _get_local_gpu_info()
cosmos_xenna-->>ClipTranscodingStage: [GpuInfo(name="...")]
ClipTranscodingStage->>cosmos_xenna: _make_gpu_resources_from_gpu_name(name)
cosmos_xenna-->>ClipTranscodingStage: GpuResources(num_nvencs=N)
ClipTranscodingStage->>Resources: Resources(nvencs=N/streams, gpu_memory_gb=M/streams)
Resources-->>ClipTranscodingStage: resources
Note over XennaAdapter: During pipeline execution
XennaAdapter->>Resources: processing_stage.resources
Resources-->>XennaAdapter: {cpus, gpus, nvdecs, nvencs, entire_gpu}
XennaAdapter->>XennaResources: XennaResources(cpus, gpus, nvdecs, nvencs, entire_gpu)
XennaResources-->>XennaAdapter: resource allocation for Ray
|
There was a problem hiding this comment.
Additional Comments (1)
-
tests/stages/video/clipping/test_clip_transcoding_stage.py, line 31-42 (link)style: Both
MockGpuInfoandMockGpuResourcesclasses are defined but never used in any tests. Consider removing them or adding tests that utilize these mocks.
7 files reviewed, 1 comment
| gpu_info = _get_local_gpu_info()[0] | ||
| nvencs = _make_gpu_resources_from_gpu_name(gpu_info.name).num_nvencs |
There was a problem hiding this comment.
logic: Accessing [0] assumes at least one GPU is available. This will raise an IndexError if no GPUs are detected. Should this handle the case where no GPUs are available or add validation?
| nvencs=nvencs // self.nb_streams_per_gpu, gpu_memory_gb=gpu_memory_gb // self.nb_streams_per_gpu | ||
| ) |
There was a problem hiding this comment.
logic: Integer division by nb_streams_per_gpu could result in zero resources if the divisor is larger than the dividend. What should happen when nb_streams_per_gpu exceeds available nvencs or gpu_memory_gb?
Greptile's behavior is changing!From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
| # Mock GPU resources class to simulate GPU resources | ||
| class MockGpuResources: | ||
| def __init__(self, num_nvencs: int = 3, num_nvdecs: int = 3): | ||
| self.num_nvencs = num_nvencs | ||
| self.num_nvdecs = num_nvdecs | ||
|
|
There was a problem hiding this comment.
style: MockGpuResources class is defined but never used in any test. Either add tests for the new nvenc-based resource allocation in ClipTranscodingStage.__post_init__ that use this mock, or remove the dead code.
Reverts #1202 due to some issues we see with Xenna's GPU usage vs the allocated resources via Ray