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

[Data] Remove _default_metadata_providers #47575

Merged
merged 3 commits into from
Sep 11, 2024
Merged

[Data] Remove _default_metadata_providers #47575

merged 3 commits into from
Sep 11, 2024

Conversation

bveeramani
Copy link
Member

Why are these changes needed?

_default_metadata_providers adds a layer of indirection.

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 added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • 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
Initial commit
90928e8 
Signed-off-by: Balaji Veeramani <[email protected]>
@bveeramani
Appease lint
0a356db 
Signed-off-by: Balaji Veeramani <[email protected]>
@bveeramani bveeramani added the go add ONLY when ready to merge, run all tests label Sep 10, 2024
omatthew98
omatthew98 approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@omatthew98 omatthew98 left a comment

Choose a reason for hiding this comment

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

One nitty naming comment, but lgtm regardless 🧹

python/ray/data/read_api.py Outdated
@@ -886,7 +887,7 @@ class string
ValueError: if ``mode`` is unsupported.
"""
if meta_provider is None:
meta_provider = get_image_metadata_provider()
meta_provider = _ImageFileMetadataProvider()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Should we rename _ImageFileMetadataProvider to ImageFileMetadataProvider since it is now being used outside image datasource?

@bveeramani
Address review comments
42c4845 
Signed-off-by: Balaji Veeramani <[email protected]>
@bveeramani bveeramani merged commit 12e1325 into master Sep 11, 2024
5 checks passed
@bveeramani bveeramani deleted the remove-default-meta-providers branch September 11, 2024 08:18
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@bveeramani @ujjawal-khare
[Data] Remove _default_metadata_providers (ray-project#47575)
386ce86 
_default_metadata_providers adds a layer of indirection.

---------

Signed-off-by: Balaji Veeramani <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@bveeramani @ujjawal-khare
[Data] Remove _default_metadata_providers (ray-project#47575)
7a70a2b 
_default_metadata_providers adds a layer of indirection.

---------

Signed-off-by: Balaji Veeramani <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@bveeramani @ujjawal-khare
[Data] Remove _default_metadata_providers (ray-project#47575)
b8c383b 
_default_metadata_providers adds a layer of indirection.

---------

Signed-off-by: Balaji Veeramani <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@bveeramani @ujjawal-khare
[Data] Remove _default_metadata_providers (ray-project#47575)
88e93eb 
_default_metadata_providers adds a layer of indirection.

---------

Signed-off-by: Balaji Veeramani <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@bveeramani @ujjawal-khare
[Data] Remove _default_metadata_providers (ray-project#47575)
34e9164 
_default_metadata_providers adds a layer of indirection.

---------

Signed-off-by: Balaji Veeramani <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@bveeramani @ujjawal-khare
[Data] Remove _default_metadata_providers (ray-project#47575)
bd62481 
_default_metadata_providers adds a layer of indirection.

---------

Signed-off-by: Balaji Veeramani <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@bveeramani @ujjawal-khare
[Data] Remove _default_metadata_providers (ray-project#47575)
65ad19f 
_default_metadata_providers adds a layer of indirection.

---------

Signed-off-by: Balaji Veeramani <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@bveeramani @ujjawal-khare
[Data] Remove _default_metadata_providers (ray-project#47575)
747b6f5 
_default_metadata_providers adds a layer of indirection.

---------

Signed-off-by: Balaji Veeramani <[email protected]>
Signed-off-by: ujjawal-khare <[email protected]>
ujjawal-khare pushed a commit to ujjawal-khare-27/ray that referenced this pull request Oct 15, 2024
@bveeramani @ujjawal-khare
[Data] Remove _default_metadata_providers (ray-project#47575)
69ca5c5 
_default_metadata_providers adds a layer of indirection.

---------

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

@omatthew98 omatthew98 omatthew98 approved these changes

@ericl ericl Awaiting requested review from ericl

@scv119 scv119 Awaiting requested review from scv119

@c21 c21 Awaiting requested review from c21

@amogkam amogkam Awaiting requested review from amogkam

@scottjlee scottjlee Awaiting requested review from scottjlee scottjlee is a code owner

@raulchen raulchen Awaiting requested review from raulchen raulchen is a code owner

@stephanie-wang stephanie-wang Awaiting requested review from stephanie-wang stephanie-wang is a code owner

Assignees

@omatthew98 omatthew98

Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

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