-
Notifications
You must be signed in to change notification settings - Fork 30
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.
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
Adding Native Dali Data Loader support for TFRecord, Images, and NPZ files #118
Conversation
@hariharan-devarajan , I pull part of your PR #81. For format, I keep only npz, npy, jpeg, png, hdf5. I don't have dlio_npz, etc. We can always determine whether it is dlio_npz or native_npz based on the data_loader selected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some potential improvements.
@hariharan-devarajan I also have to removed the cache installation for DLIO, because of the code changes does not get push to the dlio installation, causing failing tests. I think it is not good to keep cache for DLIO installation. |
@hariharan-devarajan Please review it again and check whether the changes look good to you. |
What if we split dependency installation with actual code installation? The pip way to do this is.
What do u think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more minor comments.
Can u do the review again button. The commenting doesn’t notify me correctly. |
@hariharan-devarajan Please see all the comments again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there :)
I think splitting dependency installation and actuall code installation will be good. New commit added this. commit 1f03159 and commit 5dd6ebf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me.
This PR is a partial merge of the following PR #81 @hariharan-devarajan