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] Hard deprecate DatasetPipeline #40129

Merged
merged 38 commits into from
Oct 13, 2023
Merged

[Data] Hard deprecate DatasetPipeline #40129

merged 38 commits into from
Oct 13, 2023

Conversation

bveeramani
Copy link
Member

@bveeramani bveeramani commented Oct 5, 2023 •
edited
Loading

Stacked on:

Why are these changes needed?

Fixes #40166.

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 :(

amogkam and others added 7 commits September 28, 2023 17:30
@amogkam
update
e057120 
Signed-off-by: amogkam <amogkamsetty@yahoo.com>
@amogkam
remove more
a4d9dd6 
Signed-off-by: amogkam <amogkamsetty@yahoo.com>
@amogkam
fix
ad42c6d 
Signed-off-by: amogkam <amogkamsetty@yahoo.com>
@amogkam
Merge branch 'master' of github.com:ray-project/ray into deprecate-da…
ac45905 
…taset-config

Signed-off-by: amogkam <amogkamsetty@yahoo.com>
@amogkam
fix
477127f 
Signed-off-by: amogkam <amogkamsetty@yahoo.com>
@bveeramani
Initial commit
c9abf00 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Merge branch 'pr/39963' into remove-dataset-pipeline
559ed80 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani bveeramani marked this pull request as draft October 5, 2023 00:03
@bveeramani
Remove DatasetPipeline from nightly tests
cfe4673 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Remove DatasetPipeline tests
7aff52f 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Initial commit
f5f1564 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Merge branch 'master' into remove-pipeline-nightlies
0b4378b
@bveeramani
Merge branch 'master' into remove-dataset-pipeline
116bcf9 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani bveeramani mentioned this pull request Oct 5, 2023
Closed
6 tasks
@bveeramani
Appease lint
61c2db6 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Remove pipelined tests
7fb6222 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Update remaining tests
d4ce4a0 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Remove more tests
a36afc8 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Re-add test_optimize
aea6c99 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Merge branch 'master' into remove-pipeline-tests
c8ee959
@bveeramani
Merge branch 'master' into remove-pipeline-nightlies
7ddc3ed 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Fix lint
8bf1085 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Merge branch 'master' into remove-pipeline-nightlies
b369310
@bveeramani
Appease lint
8cca990 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Merge branch 'rllib-dataset-pipeline' into remove-dataset-pipeline
c75019e 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani bveeramani marked this pull request as ready for review October 12, 2023 00:38
@bveeramani
Fix docstring issue
f0b0a50 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Merge branch 'master' of https://github.com/ray-project/ray into remo…
0e1543f 
…ve-dataset-pipeline

Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Fix repeat docstring
efecf01 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Update window
5b18b41 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Update files
ea4ff29 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Merge branch 'master' into remove-dataset-pipeline
90d1dbf 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
c21
c21 reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@c21 c21 left a comment

Choose a reason for hiding this comment

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

Thanks @bveeramani, great job!

python/ray/data/preprocessor.py Outdated Show resolved Hide resolved
python/ray/data/preprocessors/chain.py Outdated Show resolved Hide resolved
python/ray/data/dataset_pipeline.py Outdated Show resolved Hide resolved
python/ray/data/dataset.py Outdated Show resolved Hide resolved
@bveeramani
Address review comment
be004c6 
Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
@bveeramani
Merge branch 'master' of https://github.com/ray-project/ray into remo…
552224d 
…ve-dataset-pipeline

Signed-off-by: Balaji Veeramani <balaji@anyscale.com>
c21
c21 approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@c21 c21 left a comment

Choose a reason for hiding this comment

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

LGTM

scottjlee
scottjlee approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@scottjlee scottjlee left a comment

Choose a reason for hiding this comment

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

Thanks! so we will not be removing the DatasetPipeline class completely for now, just raising the deprecation warning?

@c21
Copy link
Contributor

c21 commented Oct 12, 2023

so we will not be removing the DatasetPipeline class completely for now, just raising the deprecation warning?

The only thing I am worried about is the usage of DatasetPipeline.from_iterable. I want to avoid the situation that Python throws the module/class not found, and people shocked about it. Any blocker if we keep the DatasetPipeline class around? All actual code inside DatasetPipeline is removed.

@bveeramani bveeramani merged commit 65ed62d into ray-project:master Oct 13, 2023
36 of 40 checks passed
@bveeramani bveeramani deleted the remove-dataset-pipeline branch October 13, 2023 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c21 c21 c21 approved these changes

@scottjlee scottjlee scottjlee approved these changes

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

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

@amogkam amogkam Awaiting requested review from amogkam amogkam 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

@sven1977 sven1977 Awaiting requested review from sven1977

@gjoliver gjoliver Awaiting requested review from gjoliver

@avnishn avnishn Awaiting requested review from avnishn

@ArturNiederfahrenhorst ArturNiederfahrenhorst Awaiting requested review from ArturNiederfahrenhorst

@smorad smorad Awaiting requested review from smorad

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@kouroshHakha kouroshHakha Awaiting requested review from kouroshHakha

@krfricke krfricke Awaiting requested review from krfricke

Assignees

@c21 c21

@scottjlee scottjlee

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Data] Deprecate DatasetPipeline
4 participants
@bveeramani @c21 @scottjlee @amogkam