Skip to content

perf(data): parallelize NeMo Gym image encoding - #3721

Merged
terrykong merged 3 commits into
mainfrom
codex/parallel-image-encoding
Aug 24, 2026
Merged

perf(data): parallelize NeMo Gym image encoding#3721
terrykong merged 3 commits into
mainfrom
codex/parallel-image-encoding

Conversation

@youngeunkwon0405

@youngeunkwon0405 youngeunkwon0405 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Parallelizes local-image conversion in encode_images_in_examples without changing its public API or supported payload shapes.

The function now:

  • groups image parts by their exact local source string;
  • encodes each unique source once with a bounded ThreadPoolExecutor(max_workers=8);
  • closes each decoded PIL image as soon as encoding finishes to release its pixel buffer;
  • mutates the NeMo Gym payload only on the caller thread after all unique sources have encoded successfully; and
  • reuses one data-URL string for every part sharing a source, reducing the payload serialized across the Ray boundary.

The worker count is capped at eight to bound peak decoded-image memory. In a CPU microbenchmark using 1024 x 1024 PNGs, the thread pool provided approximately 3.7-3.9x speedup. Deduplication is a separate gain: GRPO repeats each prompt per generation, so a group size of N previously encoded the same image approximately N times and now encodes it once.

HTTP(S) and existing data: URLs remain pass-through values, and local paths plus file:// URLs keep the existing PNG data-URL behavior.

Issues

None.

Validation

  • uvx --from ruff==0.9.9 ruff check nemo_rl/data/multimodal_utils.py tests/unit/data/test_multimodal_image_encoding.py
  • uvx --from ruff==0.9.9 ruff format --check nemo_rl/data/multimodal_utils.py tests/unit/data/test_multimodal_image_encoding.py
  • uv run --extra nemo_gym --group test pytest -q --noconftest tests/unit/data/test_multimodal_image_encoding.py - 7 passed on the initial patch
  • Refreshed GitHub Lint check on 634f5450fecf30ba09c6495261586e01ec370ee2 - passed, including Pyrefly

Before your PR is "Ready for review"

  • Read and followed the contributor guidelines
  • Added focused tests
  • Ran focused Linux unit tests
  • No documentation update is needed because this is an internal performance change with no new configuration or public API

Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 20, 2026

Copy link
Copy Markdown

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.

@youngeunkwon0405
youngeunkwon0405 marked this pull request as ready for review August 20, 2026 01:22
@youngeunkwon0405
youngeunkwon0405 requested review from a team as code owners August 20, 2026 01:22
@youngeunkwon0405 youngeunkwon0405 self-assigned this Aug 20, 2026
@youngeunkwon0405 youngeunkwon0405 added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label Aug 20, 2026
@youngeunkwon0405

Copy link
Copy Markdown
Contributor Author

/ok to test 6957387

Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com>
@youngeunkwon0405

Copy link
Copy Markdown
Contributor Author

/ok to test 634f545

Comment thread tests/unit/data/test_multimodal_image_encoding.py Outdated
Comment thread tests/unit/data/test_multimodal_image_encoding.py Outdated
Comment thread tests/unit/data/test_multimodal_image_encoding.py Outdated
Comment thread nemo_rl/data/multimodal_utils.py Outdated
Comment thread nemo_rl/data/multimodal_utils.py
Comment thread nemo_rl/data/multimodal_utils.py Outdated

@aroshanghias-nvd aroshanghias-nvd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One non-blocking operational concern:

Comment thread nemo_rl/data/multimodal_utils.py
Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com>
@youngeunkwon0405

Copy link
Copy Markdown
Contributor Author

/ok to test 462ff92

@DanialTaheri DanialTaheri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good to me

@DanialTaheri

DanialTaheri commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

@yfw @terrykong can we merge this? It is a harmless optimization for image encoding.

@youngeunkwon0405

Copy link
Copy Markdown
Contributor Author

Hi @terrykong, can I get your help with merging this PR? It is a ~20-line logic change that parallelizes image encoding for the VLM models. I already did it through review-pr-team, the CI run passed, and I got approval from @DanialTaheri. Thanks for your help always!

@terrykong
terrykong enabled auto-merge (squash) August 24, 2026 19:04
@terrykong
terrykong merged commit 55227be into main Aug 24, 2026
85 checks passed
@terrykong
terrykong deleted the codex/parallel-image-encoding branch August 24, 2026 19:06
yfw pushed a commit that referenced this pull request Aug 27, 2026
…3875)

Signed-off-by: Youngeun Kwon <youngeunk@nvidia.com>
Co-authored-by: Youngeun Kwon <youngeunk@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants