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-21045][PYSPARK]Fixed executor blocked because traceback.format_exc throw UnicodeDecodeError #18324

Closed
wants to merge 1 commit into from

Conversation

dataknocker
Copy link

What changes were proposed in this pull request?

check if traceback.format_exc() is unicode then encode utf8.

How was this patch tested?

We can run in pyspark:

def f():
    raise Exception("中")
spark = SparkSession.builder.master('local').getOrCreate()
spark.sparkContext.parallelize([1]).map(lambda x: f()).count()

Before fixed this bug, this program will be blocked.
After fixed this bug, this program will throw exception expected.

And I have added the test to pyspark.tests.

@dataknocker
Copy link
Author

dataknocker commented Jun 16, 2017

@HyukjinKwon pr #18262 have some problems. So I add this new pr.
I add my test for this change.


self.sc.parallelize([1]).map(lambda x: f()).count()
except Exception:
pass
Copy link
Member

Choose a reason for hiding this comment

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

I would check this with assertRaises and the error message too.

except Exception:
pass

t = threading.Thread(target=run)
Copy link
Member

Choose a reason for hiding this comment

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

Why should we run this in a thread?

Copy link
Author

@dataknocker dataknocker Jun 19, 2017

Choose a reason for hiding this comment

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

@HyukjinKwon This test mainly check whether it is blocked. So I use thread join, if it is blocked before fixing the bug the program will wait 10s and exit instead blocking other tests.

@@ -177,8 +180,11 @@ def process():
process()
except Exception:
try:
exc_info = traceback.format_exc()
if isinstance(exc_info, unicode):
exc_info = exc_info.encode('utf-8')
Copy link
Member

Choose a reason for hiding this comment

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

I guess we need to follow #17267 each other to fix correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should take a closer look. BTW, just note that, they are a bit different in that sense this one needs to return bytes in Python 3 / string (bytes) in Python 2 whereas #17267 needs to produce string (unicode) in Python 3 / string (bytes) in Python 2.

Copy link
Author

Choose a reason for hiding this comment

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

@HyukjinKwon @ueshin what need I do next?

Copy link
Member

Choose a reason for hiding this comment

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

Let's wait for the resolution of #17267 if you don't mind. I think we should be careful of this change.

Copy link
Member

Choose a reason for hiding this comment

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

cc @zero323 and @davies here too (for the approach here). This instance is a bit different.

IMHO, we have a strong assumption that the string is in UTF-8 and this PR now allows writing out the bytes as are. This is a hole which I can't come up with a clean solution to handle because this means all other encoded strings can be written up to my knowledge. Also, we have this assumption in JVM side that this is in UTF-8.

However, in Java, it mangles if it is not in UTF-8 rather than throwing an exception up to my knowledge. I guess this is still better than hanging there.

Would you have a better idea to deal with this maybe or is there anything I missed here?

Copy link
Author

Choose a reason for hiding this comment

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

@HyukjinKwon this pr can only be hanging?

Copy link
Author

Choose a reason for hiding this comment

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

@advancedxy
Copy link
Contributor

@dataknocker is there any updates on this pr?

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@HyukjinKwon
Copy link
Member

@advancedxy, would you like to take this over?

@advancedxy
Copy link
Contributor

@advancedxy, would you like to take this over?

All right, let me take this over. And hope we can get this into Spark3 and backports to 2.4

@HyukjinKwon
Copy link
Member

Closing this in favour of #25847

HyukjinKwon pushed a commit that referenced this pull request Sep 20, 2019
…from python execution in Python 2

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

This PR allows non-ascii string as an exception message in Python 2 by explicitly en/decoding in case of `str` in Python 2.

### Why are the changes needed?

Previously PySpark will hang when the `UnicodeDecodeError` occurs and the real exception cannot be passed to the JVM side.

See the reproducer as below:

```python
def f():
    raise Exception("中")
spark = SparkSession.builder.master('local').getOrCreate()
spark.sparkContext.parallelize([1]).map(lambda x: f()).count()
```

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

User may not observe hanging for the similar cases.

### How was this patch tested?

Added a new test and manually checking.

This pr is based on #18324, credits should also go to dataknocker.
To make lint-python happy for python3, it also includes a followup fix for #25814

Closes #25847 from advancedxy/python_exception_19926_and_21045.

Authored-by: Xianjin YE <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
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.

6 participants