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

Add O_DIRECT support to the TFRecord reader #4820

Merged
merged 4 commits into from
Jun 12, 2023
Merged

Add O_DIRECT support to the TFRecord reader #4820

merged 4 commits into from
Jun 12, 2023

Conversation

JanuszL
Copy link
Contributor

@JanuszL JanuszL commented May 2, 2023 •
edited
Loading

  • adds the use_o_direct option to the TFRecord reader. In effect
    the reader reads to the internal buffer which chunks are shared
    with samples. When the buffer runs out of content new one
    is allocated and the old lives as long as any sample still
    uses a piece of it

Category:

New feature (non-breaking change which adds functionality)

Description:

  • adds the use_o_direct option to the TFRecord reader. In effect
    the reader reads to the internal buffer which chunks are shared
    with samples. When the buffer runs out of content new one
    is allocated and the old lives as long as any sample still
    uses a piece of it

Additional information:

Affected modules and functionalities:

  • dali/operators/reader/loader/indexed_file_loader.h
  • dali/operators/reader/parser/tfrecord_parser.h
  • dali/operators/reader/tfrecord_reader_op.cc
  • dali/test/python/reader/test_index.py

Key points relevant for the review:

  • NA

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
      • test_index.test_tfrecord_pad_last_batch
      • test_index.test_tfrecord_odirect
    • 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-3404

@JanuszL JanuszL marked this pull request as draft May 2, 2023 08:37
@JanuszL
Copy link
Contributor Author

JanuszL commented May 2, 2023

!build

@JanuszL JanuszL changed the title Tf record odirect Add O_DIRECT support to the TFRecord reader May 2, 2023
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8147944]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8147944]: BUILD PASSED

@JanuszL
Copy link
Contributor Author

JanuszL commented May 2, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8149814]: BUILD STARTED

@JanuszL
Copy link
Contributor Author

JanuszL commented May 2, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8150431]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8150431]: BUILD FAILED

@JanuszL
Copy link
Contributor Author

JanuszL commented May 4, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8174852]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8174852]: BUILD FAILED

@JanuszL JanuszL force-pushed the tf_record_odirect branch from 88a61b2 to 7114544 Compare May 4, 2023 15:37
@JanuszL
Copy link
Contributor Author

JanuszL commented May 4, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8178581]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8178581]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8178581]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8224536]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8224536]: BUILD FAILED

@JanuszL JanuszL force-pushed the tf_record_odirect branch 2 times, most recently from 7bf4fcf to c1096c4 Compare May 15, 2023 07:04
@JanuszL
Copy link
Contributor Author

JanuszL commented May 16, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8303761]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8303761]: BUILD FAILED

@JanuszL JanuszL force-pushed the tf_record_odirect branch from 59141a2 to d1548c9 Compare May 16, 2023 09:45
@JanuszL
Copy link
Contributor Author

JanuszL commented May 16, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8303921]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8303921]: BUILD FAILED

@JanuszL JanuszL force-pushed the tf_record_odirect branch from d1548c9 to aa4780f Compare May 16, 2023 10:55
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8304874]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8304874]: BUILD PASSED

@JanuszL
Copy link
Contributor Author

JanuszL commented May 16, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8309115]: BUILD STARTED

@JanuszL JanuszL force-pushed the tf_record_odirect branch from 4a0bd07 to 8ae5221 Compare May 17, 2023 11:25
@JanuszL JanuszL marked this pull request as ready for review June 6, 2023 07:46
@JanuszL
Copy link
Contributor Author

JanuszL commented Jun 6, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8542796]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8542796]: BUILD PASSED

jantonguirao
jantonguirao reviewed Jun 6, 2023
View reviewed changes
dali/operators/reader/tfrecord_reader_op.cc Outdated
DALI_STRING_VEC);
DALI_STRING_VEC)
.AddOptionalArg("use_o_direct",
R"code(If set to True, the data will be read directly from the storage bypassing system
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
R"code(If set to True, the data will be read directly from the storage bypassing system
R"code(If set to True, the data will be read directly from the storage bypassing the system

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

jantonguirao
jantonguirao reviewed Jun 6, 2023
View reviewed changes
dali/operators/reader/loader/indexed_file_loader.h Outdated
* ----------------XXXX************XXXXXXXXXXXXXXXXXXX----------------
*/
// read again if there is no buffer of the requested piece if outside of the it
if (!read_buffer_ || !(seek_pos >= static_cast<int64>(read_buffer_pos_) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick. The content of this if statement is a bit too complex. Can we make some bool variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

jantonguirao
jantonguirao reviewed Jun 6, 2023
View reviewed changes
dali/operators/reader/loader/indexed_file_loader.h Outdated
};
// store the work lambda into queue so the prefetch thread can pick them up latter and
// execute in multiple threads
PutReadWork(work);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
PutReadWork(work);
PutReadWork(std::move(work));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

JanuszL added 3 commits June 6, 2023 13:27
@JanuszL
Add O_DIRECT support to the TFRecord reader
521edd7 
- adds the `use_o_direct` option to the TFRecord reader. In effect
  the reader reads to the internal buffer which chunks are shared
  with samples. When the buffer runs out of content new one
  is allocated and the old lives as long as any sample still
  uses a piece of it

Signed-off-by: Janusz Lisiecki <[email protected]>
@JanuszL
Parallelize reading
73a4513 
Signed-off-by: Janusz Lisiecki <[email protected]>
@JanuszL
Fix
49e36ed 
Signed-off-by: Janusz Lisiecki <[email protected]>
@JanuszL JanuszL force-pushed the tf_record_odirect branch from 8ae5221 to 49e36ed Compare June 6, 2023 11:31
@JanuszL
Review fixes
a9b400b 
Signed-off-by: Janusz Lisiecki <[email protected]>
@JanuszL
Copy link
Contributor Author

JanuszL commented Jun 6, 2023

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8545041]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8545041]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8545041]: BUILD PASSED

@JanuszL JanuszL merged commit 39f885b into NVIDIA:main Jun 12, 2023
@JanuszL JanuszL deleted the tf_record_odirect branch June 12, 2023 07:41
stiepan pushed a commit that referenced this pull request Jun 12, 2023
@JanuszL @stiepan
Add O_DIRECT support to the TFRecord reader (#4820)
ae1de76 
- adds the `use_o_direct` option to the TFRecord reader. In effect
  the reader reads to the internal buffer which chunks are shared
  with samples. When the buffer runs out of content new one
  is allocated and the old lives as long as any sample still
  uses a piece of it

Signed-off-by: Janusz Lisiecki <[email protected]>
@JanuszL JanuszL mentioned this pull request Sep 6, 2023
Closed
JanuszL added a commit to JanuszL/DALI that referenced this pull request Oct 13, 2023
@JanuszL
Add O_DIRECT support to the TFRecord reader (NVIDIA#4820)
14485db 
- adds the `use_o_direct` option to the TFRecord reader. In effect
  the reader reads to the internal buffer which chunks are shared
  with samples. When the buffer runs out of content new one
  is allocated and the old lives as long as any sample still
  uses a piece of it

Signed-off-by: Janusz Lisiecki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awolant awolant awolant approved these changes

@jantonguirao jantonguirao jantonguirao approved these changes

Assignees

@jantonguirao jantonguirao

@awolant awolant

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JanuszL @dali-automaton @jantonguirao @awolant