Skip to content

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Jul 24, 2023

What changes were proposed in this pull request?

The pr aims to balancing pyspark-pandas-connect and pyspark-pandas-slow-connect GA testing time.

Why are the changes needed?

After pr: #42146, the difference in testing time between pyspark-pandas-connect and pyspark-pandas-slow-connect is a bit significant, which affects the overall running time. In order to make GA operation more efficient and stable.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • Pass GA.
  • Manually monitor GA.

@LuciferYang
Copy link
Contributor

image

It seems that splitting the test task is useful. Do we have to split it into 3?

@LuciferYang
Copy link
Contributor

cc @HyukjinKwon @zhengruifeng FYI

@panbingkun panbingkun changed the title [DO NOT Merge][TEST] free disk space [SPARK-44524][BUILD] Add a new test group for pyspark-pandas-slow-connect module Jul 24, 2023
HyukjinKwon
HyukjinKwon previously approved these changes Jul 24, 2023
zhengruifeng
zhengruifeng previously approved these changes Jul 25, 2023
Copy link
Contributor

@zhengruifeng zhengruifeng left a comment

Choose a reason for hiding this comment

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

LGTM if CI pass

@panbingkun
Copy link
Contributor Author

Maybe splitting into three is a stable choice. Let's try splitting into two first. Yesterday, splitting into three was successful.

@LuciferYang
Copy link
Contributor

If dividing into two groups is still easy to fail, I do not oppose splitting into three groups :)

@zhengruifeng
Copy link
Contributor

If dividing into two groups is still easy to fail, I do not oppose splitting into three groups :)

If so, I suggest we'd better find better names for the groups (let's also take existing pyspark_pandas_connect into account), maybe pyspark_pandas_connect_part_xxx

@HyukjinKwon HyukjinKwon dismissed their stale review July 25, 2023 03:47

Yeah, let's probably go with #42146 way.

@panbingkun
Copy link
Contributor Author

If so, I suggest we'd better find better names for the groups (let's also take existing pyspark_pandas_connect into account), maybe pyspark_pandas_connect_part_xxx

Ok, let me try again.

@zhengruifeng
Copy link
Contributor

I think it is fine to split pyspark-pandas-slow-connect which is slow.

not sure whether they are due to the same cause, but I also see pyspark-sql, pyspark-pandas-slow failed for some reason (e.g. https://github.com/apache/spark/actions/runs/5651977344/job/15310946651)

It's kind of dangerous, since the GA recognized them as skip and made CI succeeded.
image

@LuciferYang
Copy link
Contributor

It's kind of dangerous, since the GA recognized them as skip and made CI succeeded.

...

@zhengruifeng
Copy link
Contributor

@panbingkun yes, it seems the python packaging test is the blocker, please hold on this PR, let's fix it in #42146 first.

@zhengruifeng
Copy link
Contributor

zhengruifeng commented Jul 26, 2023

@panbingkun after we disable packaging tests in pyspark-pandas-connect and pyspark-pandas-slow-connect, in https://github.com/zhengruifeng/spark/actions/runs/5664781463/job/15348941550 , the pyspark-pandas-connect took 1h, and pyspark-pandas-slow-connect took 2h.
I think it is not worthwhile to split pyspark-pandas-slow-connect any more, instead I suggest rebalancing them by moving some tests from pyspark-pandas-slow-connect to pyspark-pandas-connect.

@panbingkun
Copy link
Contributor Author

@panbingkun after we disable packaging tests in pyspark-pandas-connect and pyspark-pandas-slow-connect, in https://github.com/zhengruifeng/spark/actions/runs/5664781463/job/15348941550 , the pyspark-pandas-connect took 1h, and pyspark-pandas-slow-connect took 2h. I think it is not worthwhile to split pyspark-pandas-slow-connect any more, instead I suggest rebalancing them by moving some tests from pyspark-pandas-slow-connect to pyspark-pandas-connect.

Ok, let me turn this PR into the logic of rebalancing? 😄

@zhengruifeng
Copy link
Contributor

@panbingkun after we disable packaging tests in pyspark-pandas-connect and pyspark-pandas-slow-connect, in https://github.com/zhengruifeng/spark/actions/runs/5664781463/job/15348941550 , the pyspark-pandas-connect took 1h, and pyspark-pandas-slow-connect took 2h. I think it is not worthwhile to split pyspark-pandas-slow-connect any more, instead I suggest rebalancing them by moving some tests from pyspark-pandas-slow-connect to pyspark-pandas-connect.

Ok, let me turn this PR into the logic of rebalancing? 😄

yeah, many thanks!

@panbingkun
Copy link
Contributor Author

panbingkun commented Jul 26, 2023

@HyukjinKwon @zhengruifeng @LuciferYang
In addition, there may be a potential fix point needed in run_python_packaging_tests testing, as it may affect the final release as 2023-09-26 time approaches.
Are we submitting a PR separately? Or did you just make it on this PR?
I prefer a separate PR because it may require backport to branch-3.3 & branch-3.4.
image

description-file = README.md

@zhengruifeng
Copy link
Contributor

@panbingkun I see, what about fixing it after 3.5 is released?

@panbingkun
Copy link
Contributor Author

@panbingkun I see, what about fixing it after 3.5 is released?

Okay, fine to me.

@github-actions github-actions bot removed the INFRA label Jul 26, 2023
@panbingkun panbingkun marked this pull request as draft July 26, 2023 11:36
@panbingkun panbingkun changed the title [SPARK-44524][BUILD] Add a new test group for pyspark-pandas-slow-connect module [SPARK-44524][BUILD] Balancing pyspark-pandas-connect and pyspark-pandas-slow-connect GA testing time Jul 26, 2023
@zhengruifeng
Copy link
Contributor

please update this PR to latest master, since we just enabled > 100 test cases in 9b40c0c and 185a0a5

@panbingkun
Copy link
Contributor Author

please update this PR to latest master, since we just enabled > 100 test cases in 9b40c0c and 185a0a5

Okay, done.

@panbingkun
Copy link
Contributor Author

@zhengruifeng
After this modification, the testing time of pyspark-pandas-connect and pyspark-pandas-slow-connect has been basically balanced.

1.pyspark-pandas-connect
image

2.pyspark-pandas-slow-connect
image

@panbingkun panbingkun marked this pull request as ready for review July 27, 2023 13:49

pyspark_pandas_slow_connect = Module(
name="pyspark-pandas-slow-connect",
dependencies=[pyspark_connect, pyspark_pandas, pyspark_pandas_slow],
Copy link
Contributor

Choose a reason for hiding this comment

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

should also set dependencies=[pyspark_connect, pyspark_pandas, pyspark_pandas_slow], in the above pyspark_pandas_connect module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@panbingkun
Copy link
Contributor Author

Let me double check the testing time this time, it looks quite balanced:
1.pyspark-pandas-connect
image

2.pyspark-pandas-slow-connect
image

@panbingkun
Copy link
Contributor Author

Third run cost:
1.pyspark-pandas-connect
image

2.pyspark-pandas-slow-connect
image

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

I am good with this. cc @itholic, @ueshin and @xinrong-meng

@zhengruifeng
Copy link
Contributor

thanks, merged to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants