Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce (experimental) DALI proxy #5726

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Introduce (experimental) DALI proxy #5726

wants to merge 15 commits into from
+1,469 −69

Conversation

jantonguirao
Copy link
Contributor

@jantonguirao jantonguirao commented Nov 27, 2024 •
edited
Loading

Category:

New feature

Description:

  • DALI proxy is a new way to integrate DALI pipelines with existing torch data loading pipelines
  • The idea is that torch data processes send data to the main process, where it is processed by DALI before handling it over to the training loop.
  • The solution allows for mixing data loading from Pytorch with partial processing on DALI

Co-author: @mdabek-nvidia

Additional information:

Affected modules and functionalities:

  • Torch plugin

Key points relevant for the review:

dali/python/nvidia/dali/plugin/pytorch/proxy/__init__.py

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • [?] RST
    • [?] Jupyter
    • Other
  • N/A
    TODO

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: N/A

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [20865442]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [20865442]: BUILD PASSED

@jantonguirao jantonguirao changed the title Dali proxy2 Introduce DALI proxy Nov 28, 2024
@jantonguirao jantonguirao marked this pull request as ready for review November 28, 2024 09:21
szkarpinski
szkarpinski previously requested changes Nov 29, 2024
View reviewed changes
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
@szkarpinski
Copy link
Collaborator

I see there are no tests except for the resnet50 example. I believe we should have normal TL0 tests as well.

@szkarpinski
Copy link
Collaborator

Did you test error propagation between the processes? Multiprocessing doesn't automagically propagate exceptions afaik. Maybe we should have tests to check how are the errors in particular processes reported?

@jantonguirao jantonguirao force-pushed the dali_proxy2 branch 2 times, most recently from caf2d0b to 0472490 Compare November 29, 2024 10:28
klecki
klecki reviewed Nov 29, 2024
View reviewed changes
Copy link
Contributor

@klecki klecki left a comment

Choose a reason for hiding this comment

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

Posting few comments as the code started moving.
I also feel like we should limit the scope of the API and hide most of the implementation.

dali/python/setup.py.in Outdated Show resolved Hide resolved
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
dali/python/nvidia/dali/plugin/pytorch/__init__.py Outdated Show resolved Hide resolved
@jantonguirao jantonguirao force-pushed the dali_proxy2 branch 7 times, most recently from c4a7f74 to 1a50983 Compare December 3, 2024 17:52
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21050826]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21050826]: BUILD FAILED

@jantonguirao jantonguirao force-pushed the dali_proxy2 branch 2 times, most recently from 3bdfd37 to 014032d Compare December 3, 2024 18:32
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21052236]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [21052236]: BUILD FAILED

@szkarpinski szkarpinski requested review from klecki and removed request for klecki January 7, 2025 17:15
@jantonguirao jantonguirao requested a review from klecki January 8, 2025 08:44
@jantonguirao
Introduce DALI proxy
7a0820e 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Code review changes
a933555 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Remove nvtx import
6be6a88 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Move to torch_utils
92f1be1 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Add tests
829b053 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Code review fixes
2c3d4fa 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Invoke test_dali_proxy.py
1219e6e 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Fix
b1f7f87 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Code review fixes
bf177b7 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Add deterministic behavior
55f0591 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
documentation
8100d76 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Code review fixes
ea01c96 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Moving DALI proxy to experimental
8ccb54b 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao
Remove changes to examples from this PR
692d107 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
@jantonguirao jantonguirao mentioned this pull request Jan 8, 2025
Open
18 tasks
@jantonguirao jantonguirao changed the title Introduce DALI proxy Introduce (experimental) DALI proxy Jan 8, 2025
@jantonguirao jantonguirao dismissed stale reviews from klecki and szkarpinski January 8, 2025 08:56

Comments addressed

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [22302052]: BUILD STARTED

@jantonguirao
Usa callable signatures and separate output references
428d776 
Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szkarpinski szkarpinski szkarpinski approved these changes

@mzient mzient mzient left review comments

@klecki klecki Awaiting requested review from klecki

Assignees

@mzient mzient

@szkarpinski szkarpinski

@mdabek-nvidia mdabek-nvidia

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jantonguirao @dali-automaton @szkarpinski @klecki @mzient @mdabek-nvidia