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

[Datasets] Add read_tfrecords #28430

Merged
merged 18 commits into from
Sep 20, 2022
Merged

[Datasets] Add read_tfrecords #28430

merged 18 commits into from
Sep 20, 2022

Conversation

bveeramani
Copy link
Member

@bveeramani bveeramani commented Sep 11, 2022 •
edited
Loading

Signed-off-by: Balaji Veeramani [email protected]

Why are these changes needed?

Users want to read data stored in TFRecords. For example, see read TFRecordDataset -> ray.data.Dataset for TensorflowTrainer.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@bveeramani bveeramani marked this pull request as draft September 11, 2022 06:32
@bveeramani bveeramani changed the title [Datasets] Add read_tf_records [Datasets] Add read_tfrecords Sep 13, 2022
@bveeramani bveeramani marked this pull request as ready for review September 13, 2022 00:35
clarkzinzow
clarkzinzow reviewed Sep 15, 2022
View reviewed changes
Copy link
Contributor

@clarkzinzow clarkzinzow left a comment

Choose a reason for hiding this comment

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

This is looking great! Biggest review item is whether a parse/decode error should result in a failure + raised error or a success + ignored record; I'd heavily push for the former.

doc/source/data/creating-datasets.rst Outdated Show resolved Hide resolved
python/ray/data/datasource/tfrecords_datasource.py Show resolved Hide resolved
python/ray/data/datasource/tfrecords_datasource.py Outdated Show resolved Hide resolved
clarkzinzow
clarkzinzow reviewed Sep 15, 2022
View reviewed changes
python/ray/data/datasource/tfrecords_datasource.py Outdated Show resolved Hide resolved
doc/source/data/creating-datasets.rst Outdated Show resolved Hide resolved
doc/source/data/creating-datasets.rst Outdated Show resolved Hide resolved
clarkzinzow
clarkzinzow approved these changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@clarkzinzow clarkzinzow left a comment

Choose a reason for hiding this comment

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

Excellent work, this is awesome! 🙌

python/ray/data/datasource/tfrecords_datasource.py Outdated Show resolved Hide resolved
@clarkzinzow clarkzinzow merged commit ad15947 into ray-project:master Sep 20, 2022
@bveeramani bveeramani mentioned this pull request Sep 23, 2022
Merged
7 tasks
@bveeramani bveeramani mentioned this pull request Oct 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clarkzinzow clarkzinzow clarkzinzow approved these changes

@ericl ericl Awaiting requested review from ericl

@scv119 scv119 Awaiting requested review from scv119

@jjyao jjyao Awaiting requested review from jjyao

@jianoaix jianoaix Awaiting requested review from jianoaix

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@c21 c21 Awaiting requested review from c21

Assignees

@clarkzinzow clarkzinzow

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bveeramani @clarkzinzow