[data/preprocessors] concatenator should preserve order of concatenated #47997

martinbomio · 2024-10-11T19:34:44Z

Make concatenator preserve order.
This is a breaking change, since the Concatenator since now the expectation is that the list of columns to include is specify always, and that list will define the order of the columns in the output column.

Why are these changes needed?

See issue #47996

Related issue number

Closes #47996

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

richardliaw · 2024-10-17T01:35:18Z

python/ray/data/preprocessors/concatenator.py

+        columns: A list of columns to concatenate. The order will define the order
+            of the concatenated columns.


Suggested change

columns: A list of columns to concatenate. The order will define the order

of the concatenated columns.

columns: A list of columns to concatenate. The provided order of the columns

will be retained during concatenation.

Also - should we just keep it as include?

ok with keeping it, but just want to point out that it would be still a breaking change, and that other preporcessors use columns to define the columns that the preprocessor would be apply to

What would be breaking in particular?

it will be a required argument right now, if non is provided it will fail

discussed offline - this makes sense and columns is more consistent with other preprocessors as well.

scottjlee

Looks great overall, just a couple small nits/docs requests

scottjlee · 2024-10-17T18:27:39Z

python/ray/data/preprocessors/concatenator.py

@@ -20,6 +20,8 @@ class Concatenator(Preprocessor):
    :class:`~ray.air.util.tensor_extensions.pandas.TensorArrayElement` objects of
    shape :math:`(m,)`, where :math:`m` is the number of columns concatenated.
    The :math:`m` concatenated columns are dropped after concatenation.
+    The preprocessor preserves the order of the columns provided in the ``colummns``
+    argument and will use that order when on transform, and transform_batch.


Suggested change

argument and will use that order when on transform, and transform_batch.

argument and will use that order when calling ``transform()`` and ``transform_batch()``.

scottjlee · 2024-10-17T18:43:34Z

python/ray/data/preprocessors/concatenator.py

@@ -20,6 +20,8 @@ class Concatenator(Preprocessor):
    :class:`~ray.air.util.tensor_extensions.pandas.TensorArrayElement` objects of
    shape :math:`(m,)`, where :math:`m` is the number of columns concatenated.
    The :math:`m` concatenated columns are dropped after concatenation.
+    The preprocessor preserves the order of the columns provided in the ``colummns``


i cannot add a comment to the docstring above this line because it wasn't modified in this PR. but can we also modify this line, to indicate that only columns specified in the columns arg will be included?

Combine numeric columns into a column of type :class:`~ray.air.util.tensor_extensions.pandas.TensorDtype`.

martinbomio · 2024-10-18T22:32:16Z

@richardliaw @scottjlee adressed all your comments!

scottjlee

LGTM, one small typo

python/ray/data/preprocessors/concatenator.py

Signed-off-by: Martin Bomio <martinbomio@spotify.com>

martinbomio · 2024-10-21T22:44:54Z

@richardliaw @scottjlee do you mind merging? I don't think I can

…ed (ray-project#47997) Make concatenator preserve order. This is a breaking change, since the `Concatenator` since now the expectation is that the list of columns to include is specify always, and that list will define the order of the columns in the output column. ## Why are these changes needed? See issue ray-project#47996 ## Related issue number Closes ray-project#47996 ## Checks - [x] I've signed off every commit(by using the -s flag, i.e., `git commit -s`) in this PR. - [x] I've run `scripts/format.sh` to lint the changes in this PR. - [x] 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. - [x] 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 - [x] Unit tests - [ ] Release tests - [ ] This PR is not tested :( Signed-off-by: Martin Bomio <martinbomio@spotify.com>

…ed (ray-project#47997) Make concatenator preserve order. This is a breaking change, since the `Concatenator` since now the expectation is that the list of columns to include is specify always, and that list will define the order of the columns in the output column. ## Why are these changes needed? See issue ray-project#47996 ## Related issue number Closes ray-project#47996 ## Checks - [x] I've signed off every commit(by using the -s flag, i.e., `git commit -s`) in this PR. - [x] I've run `scripts/format.sh` to lint the changes in this PR. - [x] 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. - [x] 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 - [x] Unit tests - [ ] Release tests - [ ] This PR is not tested :( Signed-off-by: Martin Bomio <martinbomio@spotify.com> Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>

martinbomio force-pushed the martinbomio/fix-concatenator-order branch 4 times, most recently from 531a467 to b8cf5d6 Compare October 12, 2024 15:03

martinbomio marked this pull request as ready for review October 12, 2024 22:50

martinbomio requested review from hongpeng-guo, justinvyu, matthewdeng, raulchen, woshiyyya, scottjlee, bveeramani, stephanie-wang and omatthew98 as code owners October 12, 2024 22:50

anyscalesam added triage Needs triage (eg: priority, bug/not-bug, and owning component) data Ray Data-related issues labels Oct 16, 2024

richardliaw changed the title ~~fix: concatenator should preserve order of concatenated~~ [data/preprocessors] concatenator should preserve order of concatenated Oct 16, 2024

martinbomio force-pushed the martinbomio/fix-concatenator-order branch 2 times, most recently from 2f701df to 071ad2b Compare October 17, 2024 01:31

richardliaw reviewed Oct 17, 2024

View reviewed changes

martinbomio force-pushed the martinbomio/fix-concatenator-order branch from 071ad2b to 191af73 Compare October 17, 2024 01:48

martinbomio mentioned this pull request Oct 17, 2024

[Data] Concatenate preprocessor does not preserve order of concatenation #47996

Closed

martinbomio force-pushed the martinbomio/fix-concatenator-order branch 2 times, most recently from 88acac8 to a5ec10e Compare October 17, 2024 13:07

martinbomio requested a review from a team as a code owner October 17, 2024 13:07

martinbomio force-pushed the martinbomio/fix-concatenator-order branch 2 times, most recently from 351b643 to a88e22a Compare October 17, 2024 14:58

scottjlee reviewed Oct 17, 2024

View reviewed changes

martinbomio force-pushed the martinbomio/fix-concatenator-order branch from a88e22a to ddb92f4 Compare October 17, 2024 22:47

scottjlee approved these changes Oct 21, 2024

View reviewed changes

python/ray/data/preprocessors/concatenator.py Outdated Show resolved Hide resolved

fix: concatenator should preserve order of concatenated

fe8952d

Signed-off-by: Martin Bomio <martinbomio@spotify.com>

martinbomio force-pushed the martinbomio/fix-concatenator-order branch from 206fc2c to fe8952d Compare October 21, 2024 19:25

Merge branch 'master' into martinbomio/fix-concatenator-order

92201ce

richardliaw approved these changes Oct 21, 2024

View reviewed changes

scottjlee enabled auto-merge (squash) October 22, 2024 00:04

github-actions bot added the go add ONLY when ready to merge, run all tests label Oct 22, 2024

matthewdeng approved these changes Oct 22, 2024

View reviewed changes

scottjlee merged commit e1c4e58 into ray-project:master Oct 22, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[data/preprocessors] concatenator should preserve order of concatenated #47997

[data/preprocessors] concatenator should preserve order of concatenated #47997

martinbomio commented Oct 11, 2024 •

edited

Loading

richardliaw Oct 17, 2024 •

edited

Loading

richardliaw Oct 17, 2024

martinbomio Oct 17, 2024

richardliaw Oct 17, 2024

martinbomio Oct 17, 2024

richardliaw Oct 17, 2024

scottjlee left a comment

scottjlee Oct 17, 2024

scottjlee Oct 17, 2024

martinbomio Oct 18, 2024

martinbomio commented Oct 18, 2024

scottjlee left a comment

martinbomio commented Oct 21, 2024

		columns: A list of columns to concatenate. The order will define the order
		of the concatenated columns.

	argument and will use that order when on transform, and transform_batch.
	argument and will use that order when calling ``transform()`` and ``transform_batch()``.

[data/preprocessors] concatenator should preserve order of concatenated #47997

[data/preprocessors] concatenator should preserve order of concatenated #47997

Conversation

martinbomio commented Oct 11, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

richardliaw Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottjlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinbomio commented Oct 18, 2024

scottjlee left a comment

Choose a reason for hiding this comment

martinbomio commented Oct 21, 2024

martinbomio commented Oct 11, 2024 •

edited

Loading

richardliaw Oct 17, 2024 •

edited

Loading