[VL] Add uniffle integration #3767

summaryzb · 2023-11-17T14:15:19Z

What changes were proposed in this pull request?

Add uniffle integration

How was this patch tested?

Integration test with uniffle manually
When uniffle 0.8.0 release in maven, add test in gluten action flow

github-actions · 2023-11-17T14:15:37Z

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/oap-project/gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

Other pull requests

github-actions · 2023-11-17T14:15:51Z

Run Gluten Clickhouse CI

github-actions · 2023-11-20T04:41:12Z

Run Gluten Clickhouse CI

github-actions · 2023-11-20T04:57:36Z

Run Gluten Clickhouse CI

zhztheplayer · 2023-11-20T09:07:33Z

cpp/core/jni/JniWrapper.cc

+    // rename CelebornClient RssClient
+    std::shared_ptr<CelebornClient> celebornClient =
+        std::make_shared<CelebornClient>(vm, partitionPusher, unifflePushPartitionDataMethod);
+    partitionWriterCreator = std::make_shared<CelebornPartitionWriterCreator>(std::move(celebornClient));


Would you explain a bit why Celeborn's code is reused here?

CelebornClient wraps in the partitionWriterCreator mainly do the work related to take out partition data, when evictPartitions is called. That will be all the same with different rss framework

zhztheplayer · 2023-11-20T09:10:53Z

Integration test with uniffle manually

gluten-celeborn has a GHA action job here. Is it possible for gluten-uniffle to add a similar one? Another option is to add some UTs, at least cover the newly added code.

summaryzb · 2023-11-20T10:00:48Z

I'll look into the GHA action, add one in this patch later

rhh777 · 2024-01-10T09:37:13Z

@summaryzb The org.apache.spark.shuffle.UniffleColumnarBatchSerializer class is missing in the commit?

zhztheplayer · 2024-01-10T10:01:35Z

UniffleColumnarBatchSerializer

Is it possible to reuse ColumnarBatchSerializer somehow ?

github-actions · 2024-01-11T12:51:04Z

Run Gluten Clickhouse CI

summaryzb · 2024-01-11T13:14:37Z

Is it possible to reuse ColumnarBatchSerializer somehow ?

Yes, it's almost the same. Change it to reuse ColumnarBatchSerializer

github-actions · 2024-01-11T13:15:52Z

Run Gluten Clickhouse CI

github-actions · 2024-01-15T03:59:42Z

Run Gluten Clickhouse CI

github-actions · 2024-01-15T06:21:47Z

Run Gluten Clickhouse CI

github-actions · 2024-01-15T09:14:47Z

Run Gluten Clickhouse CI

github-actions · 2024-01-15T12:35:58Z

Run Gluten Clickhouse CI

zhouyuan · 2024-01-22T03:21:24Z

@PZD-CHINA @PHILO-HE
we may need to also add Uniffle installation in docker, similar as Celeborn
https://github.com/oap-project/gluten/blob/main/tools/gluten-te/ubuntu/dockerfile-buildenv#L118

...fle/velox/src/main/java/org/apache/spark/shuffle/gluten/uniffle/GlutenRssShuffleManager.java

pom.xml

PHILO-HE · 2024-01-22T03:41:25Z

@PZD-CHINA @PHILO-HE we may need to also add Uniffle installation in docker, similar as Celeborn https://github.com/oap-project/gluten/blob/main/tools/gluten-te/ubuntu/dockerfile-buildenv#L118

@summaryzb, suppose uniffle-0.8.0 should be pre-installed in CI docker. Please let us know if there is any other dependency required. Thanks!

cpp/core/jni/JniWrapper.cc

github-actions · 2024-03-11T01:43:52Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

github-actions · 2024-04-03T06:34:41Z

Run Gluten Clickhouse CI

github-actions · 2024-04-03T09:20:29Z

Run Gluten Clickhouse CI

github-actions · 2024-04-03T09:20:42Z

Run Gluten Clickhouse CI

summaryzb · 2024-04-03T11:56:35Z

@PHILO-HE PTAL

summaryzb · 2024-04-03T11:59:59Z

Uniffle run tpcds with only one worker may encounter some stability issues occasionally，so this integration test only run with tpch

summaryzb · 2024-04-06T01:32:21Z

@jackylee-ch PTAL

PHILO-HE

Thanks for your work! Please also respond to Hongze's comments. Thanks!
#3767 (comment)

.github/workflows/velox_docker.yml

PHILO-HE · 2024-04-07T07:54:11Z

cpp/core/jni/JniWrapper.cc

+    // rename CelebornClient RssClient
+    std::shared_ptr<CelebornClient> uniffleClient =
+        std::make_shared<CelebornClient>(vm, partitionPusher, unifflePushPartitionDataMethod);
+    partitionWriter = std::make_unique<CelebornPartitionWriter>(


Like the above comment, better to use a common name if they indeed can be shared by both celeborn & uniffle.

Using a common name relate to all the logic of rss in jni layer, but not limit to this PartitionWriter, maybe it's better to resolve this in a separate pr

@summaryzb, ok to me. Please create an issue to track this. Thanks!

Follow this

...fle/velox/src/main/java/org/apache/spark/shuffle/gluten/uniffle/GlutenRssShuffleManager.java

github-actions · 2024-04-09T02:10:27Z

Run Gluten Clickhouse CI

github-actions · 2024-04-09T03:10:20Z

Run Gluten Clickhouse CI

github-actions · 2024-04-09T05:13:38Z

Run Gluten Clickhouse CI

summaryzb · 2024-04-09T08:23:30Z

@PHILO-HE PTAL

pom.xml

PHILO-HE

@zhztheplayer, do you have any other comment?

PHILO-HE · 2024-04-10T03:24:24Z

...e/velox/src/main/java/org/apache/spark/shuffle/writer/VeloxUniffleColumnarShuffleWriter.java

+            sparkConf.getSizeAsBytes(
+                RssSparkConfig.RSS_WRITER_BUFFER_SIZE.key(),
+                RssSparkConfig.RSS_WRITER_BUFFER_SIZE.defaultValue().get());
+    compressionCodec = GlutenShuffleUtils.getCompressionCodec(sparkConf);


I note celeborn honors Spark's config for SHUFFLE_COMPRESS. Should uniffle do the same check? See e182d65.

github-actions · 2024-04-10T12:46:05Z

Run Gluten Clickhouse CI

zhztheplayer · 2024-04-11T03:16:36Z

@zhztheplayer, do you have any other comment?

I'm good if we have tests covering the feature. Please proceed if it's ready to merge.

Thanks @summaryzb for this work!

jackylee-ch

Since we have a follow-up issue #5335 to solve the conflicting code, there is no problem on my side. Thanks for your work.@summaryzb

jackylee-ch · 2024-04-11T04:07:29Z

This pr is ready to be merged on my side. Would leave some time to see if there are any more suggestions.

zhztheplayer

@summaryzb Thank you for working on uniffle integration and I just left some late minor review comments.

zhztheplayer · 2024-04-19T06:57:22Z

.github/workflows/velox_docker.yml

+        run: |
+          export MAVEN_HOME=/usr/lib/maven && \
+          export PATH=${PATH}:${MAVEN_HOME}/bin && \
+          export export JAVA_HOME=/usr/lib/jvm/java-1.8.0-openjdk && \


export export -> export

zhztheplayer · 2024-04-19T06:58:05Z

.github/workflows/velox_docker.yml

+          tar xzf apache-uniffle-0.8.0-incubating-bin.tar.gz -C /opt/ && mv /opt/rss-0.8.0-hadoop2.8 /opt/uniffle && \
+          wget -nv https://archive.apache.org/dist/hadoop/common/hadoop-2.8.5/hadoop-2.8.5.tar.gz && \
+          tar xzf hadoop-2.8.5.tar.gz -C /opt/
+          rm -f /opt/uniffle/jars/server/shuffle-server-0.8.0-SNAPSHOT.jar


Did you want to remove shuffle-server-0.8.0.jar instead of shuffle-server-0.8.0-SNAPSHOT.jar, was this a typo or something?

zhztheplayer reviewed Nov 20, 2023

View reviewed changes

summaryzb force-pushed the main branch from e96f2db to 6ebb926 Compare January 11, 2024 12:50

summaryzb force-pushed the main branch from ff445c9 to 97b23b1 Compare January 15, 2024 03:59

summaryzb force-pushed the main branch from 719e5b5 to 726fd7b Compare January 15, 2024 12:35

zhztheplayer reviewed Jan 22, 2024

View reviewed changes

...fle/velox/src/main/java/org/apache/spark/shuffle/gluten/uniffle/GlutenRssShuffleManager.java Outdated Show resolved Hide resolved

zhztheplayer reviewed Jan 22, 2024

View reviewed changes

pom.xml Show resolved Hide resolved

zhouyuan reviewed Jan 25, 2024

View reviewed changes

cpp/core/jni/JniWrapper.cc Show resolved Hide resolved

weiting-chen mentioned this pull request Feb 21, 2024

Gluten Roadmap in 2024 #4709

Open

21 tasks

github-actions bot added the stale stale label Mar 11, 2024

summaryzb closed this Mar 11, 2024

summaryzb force-pushed the main branch from 726fd7b to e78ee43 Compare March 11, 2024 05:09

summaryzb reopened this Mar 11, 2024

summaryzb force-pushed the main branch from e006dc1 to 4a393ee Compare April 3, 2024 06:34

summaryzb force-pushed the main branch 2 times, most recently from a1a5e24 to e5310aa Compare April 3, 2024 09:20

PHILO-HE reviewed Apr 7, 2024

View reviewed changes

[VL] Add uniffle integration

d312210

summaryzb force-pushed the main branch from e5310aa to d312210 Compare April 9, 2024 02:09

remove unused

6c7d4d8

format

c3b542b

summaryzb mentioned this pull request Apr 9, 2024

[VL] Use common name for both celeborn and uniffle #5335

Closed

jackylee-ch reviewed Apr 9, 2024

View reviewed changes

pom.xml Show resolved Hide resolved

PHILO-HE reviewed Apr 10, 2024

View reviewed changes

summaryzb requested review from PHILO-HE and zhztheplayer April 10, 2024 08:06

Check spark.shuffle.compress

1b6b9e4

jackylee-ch approved these changes Apr 11, 2024

View reviewed changes

jackylee-ch merged commit b9408f9 into apache:main Apr 11, 2024
38 checks passed

zhztheplayer reviewed Apr 19, 2024

View reviewed changes

[VL] Add uniffle integration #3767

[VL] Add uniffle integration #3767

Conversation

summaryzb commented Nov 17, 2023

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Nov 17, 2023

github-actions bot commented Nov 17, 2023

github-actions bot commented Nov 20, 2023

github-actions bot commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhztheplayer commented Nov 20, 2023

summaryzb commented Nov 20, 2023

rhh777 commented Jan 10, 2024

zhztheplayer commented Jan 10, 2024

github-actions bot commented Jan 11, 2024

summaryzb commented Jan 11, 2024 • edited Loading

github-actions bot commented Jan 11, 2024

github-actions bot commented Jan 15, 2024

github-actions bot commented Jan 15, 2024

github-actions bot commented Jan 15, 2024

github-actions bot commented Jan 15, 2024

zhouyuan commented Jan 22, 2024

PHILO-HE commented Jan 22, 2024

github-actions bot commented Mar 11, 2024

github-actions bot commented Apr 3, 2024

github-actions bot commented Apr 3, 2024

github-actions bot commented Apr 3, 2024

summaryzb commented Apr 3, 2024

summaryzb commented Apr 3, 2024

summaryzb commented Apr 6, 2024

PHILO-HE 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

Choose a reason for hiding this comment

github-actions bot commented Apr 9, 2024

github-actions bot commented Apr 9, 2024

github-actions bot commented Apr 9, 2024

summaryzb commented Apr 9, 2024

PHILO-HE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 10, 2024

zhztheplayer commented Apr 11, 2024

jackylee-ch left a comment

Choose a reason for hiding this comment

jackylee-ch commented Apr 11, 2024

zhztheplayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

summaryzb commented Jan 11, 2024 •

edited

Loading