-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[data] move many datasource classes for internal #46169
Conversation
9425e8e
to
3ee0736
Compare
@@ -1,12 +1,14 @@ | |||
import logging |
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.
For consistency with the Datasource
implementations, could we move the Datasink
implementations to _internal
and also remove leading underscores from the names (e.g., move ray.data.datasource.mongo_datasink._MongoDatasink
to ray.data._internal.datasource.mongo_datasink.MongoDatasink
?
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.
will do
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.
python/ray/data/read_api.py
Outdated
Connection, | ||
CSVDatasource, | ||
Datasource, | ||
ImageDatasource, |
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.
ImageDatasource
, LancecDatasource
, TFRecordDatasource
, and TorchDatasource
are annotated as developer APIs but they should be internal. Could we remove the annotations and also move them to _internal
? (Saw there's already a PR for TFRecordDatasource
: #46171)
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.
absolutely
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.
actually if it's ok let me do it in a follow up PR to reduce the size of this one
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.
Move datasink classes to internal, as discussed in #46169 Test: - CI --------- Signed-off-by: can <can@anyscale.com>
Signed-off-by: can <can@anyscale.com>
Move three developerapi datasource classes to internal Test: - CI --------- Signed-off-by: can <can@anyscale.com>
Move many data source classes internal, according to https://docs.google.com/spreadsheets/d/1wQALnsUuFN4dMgmGITfiVX41fCc6P6FEjYVO6jDWGtM/edit#gid=1683753102
Test: