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

Remove stages from Pipeline API #5244

Merged
merged 15 commits into from
Jan 29, 2024
Merged

Remove stages from Pipeline API #5244

merged 15 commits into from
Jan 29, 2024

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Dec 13, 2023 •
edited
Loading

Category:

New feature (non-breaking change which adds functionality)
Refactoring (Redesign of existing code that doesn't affect functionality)

Description:

What

Get rid of separate RunCPU/RunGPU from Pipeline and Executor interface

Why

Because it doesn't make sense to have separate APIs for running stages when we want to get rid of stages (or, if you prefer, have a flexible number of stages)

Why we can do it

These APIs are NOT present in Python or C and C++ is not official/stable/whatever

How

  • Remove the APIs from Pipeline entirely
  • Add a new Prefetch API that fills the queues according to the buffer sizes
  • Add a new feed_input_count API that tells the user/frontend how many times to fill a buffer for a particular input (much easier to use and harder to get wrong with separated queues - we were using it incorrectly before!)
  • Hide the prefetching pattern from the user/frontend behind Prefetch and feed_input_count
  • Remove the APIs from the executor's interface
  • Keep the APIs in the executor as private members (they are invoked from Run and Prefetch)

Additional information:

Affected modules and functionalities:

Executor, Pipeline, Python frontend for the pipeline, C API, TF plugin

Key points relevant for the review:

Tests:

Existing tests apply - ALL OF THEM except those that called the removed APIs from C++ directly.

  • 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

DALI team only

Requirements

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

REQ IDs: N/A

JIRA TASK: DALI-3743

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11867574]: BUILD STARTED

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 3, 2024
View reviewed changes
dali/python/nvidia/dali/pipeline.py Fixed Show fixed Hide fixed
dali/python/nvidia/dali/pipeline.py Fixed Show fixed Hide fixed
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11867574]: BUILD FAILED

szalpal
szalpal reviewed Jan 4, 2024
View reviewed changes
include/dali/c_api.h
* @return The number of calls to be made
*/
DLL_PUBLIC int
daliInputFeedCount(daliPipelineHandle *pipe_handle, const char *input_name);
Copy link
Member

Choose a reason for hiding this comment

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

Since this corresponds to the daliSetExternalInput... function set, maybe it would be good to call it daliExternalInputFeedCount? Alternatively, if this function can be used on any input (not only the external one), maybe the documentation should be modified, since now it points to the daliSetExternal... function only?

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'm not entirely satisfied with this name. I was thinking about something like InputPrefetchCount or InputPrefetchFeedCount.

Alternatively, if this function can be used on any input (not only the external one),

What other inputs are there? Everything that's not external just feeds itself. Even if you called it on some other operator, the information would be non-actionable and even misleading.

dali/pipeline/pipeline.h Outdated
Comment on lines 778 to 779
* @param fill_queue If true, the inputs are fed `InputFeedCount(name)` times;
* otherwise it's fed once.
Copy link
Member

Choose a reason for hiding this comment

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

Should it be plural?

Suggested change
* @param fill_queue If true, the inputs are fed `InputFeedCount(name)` times;
* otherwise it's fed once.
* @param fill_queue If true, the inputs are fed `InputFeedCount(name)` times;
* otherwise they're fed once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I guess...

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11881000]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11881000]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11886704]: BUILD STARTED

@mzient mzient marked this pull request as ready for review January 4, 2024 16:33
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11888104]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11886704]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11888104]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11907275]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11907275]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11908302]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11908302]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11961717]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [11961717]: BUILD PASSED

@NVIDIA NVIDIA deleted a comment from dali-automaton Jan 9, 2024
mzient
mzient commented Jan 9, 2024
View reviewed changes
dali/pipeline/executor/executor.h
Comment on lines -74 to -76
DLL_PUBLIC virtual void RunCPU() = 0;
DLL_PUBLIC virtual void RunMixed() = 0;
DLL_PUBLIC virtual void RunGPU() = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These functions go away from the interface...

mzient
mzient commented Jan 9, 2024
View reviewed changes
dali/pipeline/executor/executor.h
Comment on lines +169 to +171
DLL_PUBLIC virtual void RunCPU();
DLL_PUBLIC virtual void RunMixed();
DLL_PUBLIC virtual void RunGPU();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

...and re-emerge as an implementation detail.

stiepan
stiepan reviewed Jan 10, 2024
View reviewed changes
Copy link
Member

@stiepan stiepan left a comment

Choose a reason for hiding this comment

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

I have yet to read a few files, but I am leaving a few comment that crossed my mind so far.

dali/c_api/c_api.cc Outdated Show resolved Hide resolved
dali/python/nvidia/dali/pipeline.py Outdated Show resolved Hide resolved
dali/python/nvidia/dali/pipeline.py Show resolved Hide resolved
@szkarpinski szkarpinski mentioned this pull request Jan 23, 2024
Merged
18 tasks
@mzient
[WIP] Remove stage APIs.
e275448 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Add Pipeline::InputFeedCount.
9197891 
Add Pipeline::Prefetch.
Rework prefetching mechanism in Python.
Fix tests.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Bug fixes, python stuff.
2cc317f 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Fingers crossed.
c88dc9a 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Remove debugging code.
771f55b 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Remove unused variable; improve documentation.
fca7837 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Numerous fixes around prefetching in Python.
131b978 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Typos.
28dad93 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Fix parallel external source.
e57e8ba 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Use one fill depth due to batch size lookahead.
f841643 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Fix GCC build.
c935f9d 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Fix operator tests.
bf6be68 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Fix accidentally removed lines.
ae636d9 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Fix accidentally removed lines.
968a043 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@mzient
Review fixes.
dbf894f 
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12427612]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12427612]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12427612]: BUILD PASSED

@mzient mzient merged commit 6fec3f1 into NVIDIA:main Jan 29, 2024
6 checks passed
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [12433588]: BUILD STARTED

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szalpal szalpal szalpal left review comments

@stiepan stiepan stiepan approved these changes

@awolant awolant awolant approved these changes

Assignees

@stiepan stiepan

@awolant awolant

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mzient @dali-automaton @szalpal @stiepan @awolant