Skip to content

Conversation

@suiyoubi
Copy link
Contributor

@suiyoubi suiyoubi commented Oct 23, 2025

Description

Linked issue: #1201

For Video Transcoding stage, use similar logic as https://github.com/nvidia-cosmos/cosmos-curate/blob/main/cosmos_curate/pipelines/video/clipping/clip_extraction_stages.py#L198 to partition gpu based on _nb_streams_per_gpu.

Usage

# Add snippet demonstrating usage

Checklist

  • I am familiar with the Contributing Guide.
  • New or Existing tests cover these changes.
  • The documentation is up to date with these changes.

Signed-off-by: Ao Tang <[email protected]>
@copy-pr-bot
Copy link

copy-pr-bot bot commented Oct 23, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +69 to +70
if self.entire_gpu:
self.gpus = 1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: When entire_gpu=True, self.gpus is set to 1.0 after validation checks. If user also sets gpu_memory_gb > 0, the validation on line 52 will fail before reaching this code, which is correct. However, if user sets both entire_gpu=True and gpus > 0, this assignment will silently overwrite their explicit gpus value without raising an error. Should there be a validation check to prevent setting both entire_gpu=True and gpus > 0 explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly think the best is to drop entire_gpu and only provide gpu_memory_gb and gpus. @ayushdg @praateekmahajan what do you think

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this, entire_gpu has been confusing for me the whole time it existed. I think it was there for nvenc/nvdec reasons (which I never understood well). So if we're nuking nvenc/nvdec support this should be good to go

@abhinavg4

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed let's just drop this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make a seperate issue for this to track. Let's keep it seperate. Wanna make sure we do it properly across documentation and code and stuff

Signed-off-by: Ao Tang <[email protected]>
@suiyoubi
Copy link
Contributor Author

/ok to test d9f705a

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 7, 2026

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".

@abhinavg4 abhinavg4 merged commit c4805ae into main Jan 7, 2026
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gpuci Run GPU CI/CD on PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants