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.

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

[SPARK-50909][PYTHON][4.0] Setup faulthandler in PythonPlannerRunners #49635

Closed

Conversation

ueshin
Copy link
Member

@ueshin ueshin commented Jan 24, 2025

What changes were proposed in this pull request?

This is a backport of #49592.

Setups faulthandler in PythonPlannerRunners.

It can be enabled by the same config as UDFs.

  • SQL conf: spark.sql.execution.pyspark.udf.faulthandler.enabled
  • It fallback to Spark conf: spark.python.worker.faulthandler.enabled
  • False by default

Why are the changes needed?

The faulthandler is not set up in PythonPlannerRunners.

Does this PR introduce any user-facing change?

When enabled, if Python worker crashes, it may generate thread-dump in the error message on the best-effort basis of Python process.

How was this patch tested?

Added the related tests.

Was this patch authored or co-authored using generative AI tooling?

No.

### What changes were proposed in this pull request?

Setups `faulthandler` in `PythonPlannerRunner`s.

It can be enabled by the same config as UDFs.

- SQL conf: `spark.sql.execution.pyspark.udf.faulthandler.enabled`
- It fallback to Spark conf: `spark.python.worker.faulthandler.enabled`
- `False` by default

### Why are the changes needed?

The `faulthandler` is not set up in `PythonPlannerRunner`s.

### Does this PR introduce _any_ user-facing change?

When enabled, if Python worker crashes, it may generate thread-dump in the error message on the best-effort basis of Python process.

### How was this patch tested?

Added the related tests.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes apache#49592 from ueshin/issues/SPARK-50909/faulthandler.

Authored-by: Takuya Ueshin <[email protected]>
Signed-off-by: Takuya Ueshin <[email protected]>
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Since one of the failures is Python part, could you re-trigger once more, @ueshin ?

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 25, 2025

Oh, according to the log, pyspark.ml.tests.connect.test_parity_torch_data_loader seems to hang consequtively twice on this change. Is there any difference from master branch?

Finished test(python3.11): pyspark.ml.tests.connect.test_parity_regression (26s)
Starting test(python3.11): pyspark.ml.tests.connect.test_parity_torch_data_loader (temp output: /__w/apache-spark/apache-spark/python/target/f869134c-ed13-471f-aa7e-37562cf80415/python3.11__pyspark.ml.tests.connect.test_parity_torch_data_loader__nrhuie5k.log)
Error: The operation was canceled.

@ueshin
Copy link
Member Author

ueshin commented Jan 25, 2025

@dongjoon-hyun Actually the ml related tests can't pass on the GHA in my repo for some reason, as I also mentioned before.
Do I need any config in my repo? Are there any config changes I missed?

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 25, 2025

Oh, for me, it works fine until now, even in my Today's PR.

Starting test(python3.11): pyspark.ml.tests.connect.test_parity_torch_data_loader (temp output: /__w/spark/spark/python/target/cb298ec7-7562-4471-8d9a-6dd7d24d68bc/python3.11__pyspark.ml.tests.connect.test_parity_torch_data_loader__n4sh0od6.log)
Finished test(python3.11): pyspark.ml.tests.connect.test_parity_torch_data_loader (116s)

I also don't have a special configuration. It's a simple fork like the other contributors.

Do you happen to know someone who has the same symptom?

@ueshin
Copy link
Member Author

ueshin commented Jan 25, 2025

Trying on master with an empty commit at #49664.

I remember @HyukjinKwon said it sometimes doesn't pass in his repo as well. sometimes he said vs. always in my repo, though.

@HyukjinKwon
Copy link
Member

yeah. it's known. I have seen those test failures ONLY in fork repos..

@HyukjinKwon
Copy link
Member

Let's merge this in first. I will monitor the build.

HyukjinKwon pushed a commit that referenced this pull request Jan 25, 2025
### What changes were proposed in this pull request?

This is a backport of #49592.

Setups `faulthandler` in `PythonPlannerRunner`s.

It can be enabled by the same config as UDFs.

- SQL conf: `spark.sql.execution.pyspark.udf.faulthandler.enabled`
- It fallback to Spark conf: `spark.python.worker.faulthandler.enabled`
- `False` by default

### Why are the changes needed?

The `faulthandler` is not set up in `PythonPlannerRunner`s.

### Does this PR introduce _any_ user-facing change?

When enabled, if Python worker crashes, it may generate thread-dump in the error message on the best-effort basis of Python process.

### How was this patch tested?

Added the related tests.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #49635 from ueshin/issues/SPARK-50909/4.0/faulthandler.

Lead-authored-by: Takuya Ueshin <[email protected]>
Co-authored-by: Takuya UESHIN <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants