[SPARK-38870][SQL][PYSPARK] Make SparkSession.builder return new builder by FurcyPin · Pull Request #36161 · apache/spark

FurcyPin · 2022-04-12T10:14:25Z

What changes were proposed in this pull request?

In Scala, SparkSession.builder returns a new builder each time,
but in Python, it returns the same builder every time, with the
same static options.

Because of this, whenever SparkSession.builder.getOrCreate() is called,
the options set in the very first builder are reapplied.

To fix this, I introduce a @classproperty decorator, that is similar
to the @Property decorator, except that it works with class methods.
Now, whenever SparkSession.builder is called, a new builder is
returned as expected.

I also made the parameter Builder._options non-static, and I removed
Builder._sc which didn't seem used anymore.

Builder._sc was introduced in this commit:
58419b9

And it's only use seems to have been removed by this commit:
88696eb

How was this patch tested?

I updated the Doctests for SparkSession.Builder.getOrCreate and added
Doctests on the classproperty decorator as well.

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

AmplabJenkins · 2022-04-12T16:56:42Z

Can one of the admins verify this patch?

HyukjinKwon · 2022-04-14T00:41:25Z

Looks pretty good. Thanks - this can be a pretty important bug fix in some usecases I believe.

cc @ueshin @viirya @BryanCutler @dongjoon-hyun FYI

viirya · 2022-04-14T01:17:38Z

viirya

This looks good. Wondering why we don't have related bug report before.

FurcyPin · 2022-04-14T07:27:03Z

This looks good. Wondering why we don't have related bug report before.

Yeah, I am amazed that it hasn't been spotted earlier ^ ^

dongjoon-hyun · 2022-04-14T22:32:20Z

Could you make it as an IDed TODO, @FurcyPin ?
For example, we recommend to use a pattern TODO(SPARK-XXX) with a concrete JIRA ID.

Sure. Done.
https://issues.apache.org/jira/browse/SPARK-38912

dongjoon-hyun · 2022-04-14T22:33:22Z

dongjoon-hyun

+1, LGTM with minor comments.

HyukjinKwon

Okay, I am fine as is. LGTM2

ueshin

LGTM. I like returning a different instance each time.

HyukjinKwon · 2022-04-15T00:07:55Z

@FurcyPin mind enabling https://github.com/FurcyPin/spark/actions/workflows/build_and_test.yml? Apache Spark uses the test results in PR author's forked repository. After enabling it, you might need to rebase it.

BryanCutler

LGTM

FurcyPin · 2022-04-15T07:53:14Z

@FurcyPin mind enabling https://github.com/FurcyPin/spark/actions/workflows/build_and_test.yml? Apache Spark uses the test results in PR author's forked repository. After enabling it, you might need to rebase it.

Sure, done.

FurcyPin · 2022-04-15T15:14:46Z

It looks like the pipeline failed with the following error :

Traceback (most recent call last):
  File "/usr/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/__w/spark/spark/python/pyspark/sql/session.py", line 1227, in <module>
    _test()
  File "/__w/spark/spark/python/pyspark/sql/session.py", line 1216, in _test
    (failure_count, test_count) = doctest.testmod(
  File "/usr/lib/python3.9/doctest.py", line 1955, in testmod
    for test in finder.find(m, name, globs=globs, extraglobs=extraglobs):
  File "/usr/lib/python3.9/doctest.py", line 939, in find
    self._find(tests, obj, name, module, source_lines, globs, {})
  File "/usr/lib/python3.9/doctest.py", line 1001, in _find
    self._find(tests, val, valname, module, source_lines,
  File "/usr/lib/python3.9/doctest.py", line 1028, in _find
    val = getattr(obj, valname).__func__
AttributeError: 'Builder' object has no attribute '__func__'

It's quite strange, I can't reproduce on my side. I'm using Python 3.9.12 and I see the pipeline used Python 3.9.5.
Maybe it's related ?
I won't have the time to investigate further until Tuesday, but if someone have any pointer, I'll be glad to hear it.

HyukjinKwon · 2022-04-18T03:55:10Z

Yeah, looks like Python version problem. We might have to upgrade Python 3.9 maintenance version.

HyukjinKwon · 2022-04-18T03:56:07Z

For now, maybe you can comment this branch out, and create another JIRA to upgrade Python version in CI.

BTW, it would be great if we can actually test and verify that it's Python version problem.

dongjoon-hyun · 2022-04-19T06:41:51Z

+1 for @HyukjinKwon 's suggestion about the verification.

To @FurcyPin , if this patch broke old Python 3.9.5, that could be considered a regression by the production users.

I'm using Python 3.9.12 and I see the pipeline used Python 3.9.5.

FurcyPin · 2022-04-19T09:06:17Z

Okay, I instally Python 3.9.5 manually on my laptop, and I confirm that the unit tests are failing with this version, and that they were not failing with Python 3.9.12.

I will try to find a fix.

FurcyPin · 2022-04-19T09:53:47Z

I think this is the bug I'm running into: python/cpython#28838

Basically, after they made @classmethod and @Property compatible, it caused docstrings to call ._func_ systematically.

A fix has been backported in 3.9 and 3.10. I believe 3.9.6 has the fix.

I see several possible solution:

Add an artificial _func_ hoping it will work around
Wait for the python version in the CI to be bumped to 3.9.6 or higher
Change my if statement like this :

if sys.version_info > (3, 9, 5)

What do you think ?

HyukjinKwon · 2022-04-19T10:58:07Z

I think you can just file a JIRA to upgrade the Python version in the CI, and just remove

        @classmethod
        @property
        def builder(cls):
            """A class attribute having a :class:`Builder` to construct :class:`SparkSession` instances."""
            return cls.Builder()

for now. We will likely bring it back before Spark 3.4.0 release in any event. Your 3. works too.

FurcyPin · 2022-04-20T08:12:28Z

Okay,

I removed the @classmethod + @Property for now, and only use @classproperty for all Python versions.
I updated https://issues.apache.org/jira/browse/SPARK-38912 to reflect the change
I create https://issues.apache.org/jira/browse/SPARK-38964 to bump version of Python 3.9 in CI

FurcyPin · 2022-04-20T14:35:20Z

I fixed the linter's warnings and I also update SparkSession.builder's docstring to make it reflect the change of behaviour
and look more like Scala's SparkSession.builder description.

FurcyPin · 2022-04-22T11:04:25Z

@HyukjinKwon
I couldn't make all checks pass as I keep being bullied by the python linter.
I had to silence one warning because I couldn't find any way to fix it.
But now the build fails on a file that I didn't even touch...

examples/src/main/python/pagerank.py:79: error: unused "type: ignore" comment

This is referring to this line, which I did not even touch:

spark/examples/src/main/python/pagerank.py

Line 79 in 80929d6

url_urls_rank[1][0], url_urls_rank[1][1] # type: ignore[arg-type]

I don't know what to do. Could you please help me ?

HyukjinKwon · 2022-04-25T00:55:05Z

Weird. That looks unrelated to change. Mind rebasing your master branch? Maybe there are some mypy version pinned in GitHub Actions workflow that cannot be synced when running tests. (thus it's dependent on master branch in your fork)

## What changes were proposed in this pull request? In Scala, SparkSession.builder returns a new builder each time, but in Python, it returns the same builder every time, with the same static options. Because of this, whenever SparkSession.builder.getOrCreate() is called, the options set in the very first builder are reapplied. To fix this, I introduce a @classproperty decorator, that is similar to the @Property decorator, except that it works with class methods. Now, whenever `SparkSession.builder` is called, a new builder is returned as expected. I also made the parameter `Builder._options` non-static, and I removed `Builder._sc` which didn't seem used anymore. `Builder._sc` was introduced in this commit: 58419b9 And it's only use seems to have been removed by this commit: 88696eb ## How was this patch tested? I updated the Doctests for SparkSession.Builder.getOrCreate and added Doctests on the classproperty decorator as well.

## Update of previous commit Since Python 3.9, the @classmethod decorator is compatible with the @Property decorator. This change adds a check on the Python version to use the "proper" way in 3.9 and beyond, and a TODO to remember cleaning up once support for Python 3.8 is dropped.

## Update of previous commits Update docstring for SparkSession.builder, to make it match with Scala's description.

FurcyPin · 2022-04-26T08:23:24Z

@HyukjinKwon I just rebased and it's still happening.

I did not have this error (but I had other errors) before this commit

Maybe for some obscure reason, the fact that I added # type: ignore in session.py triggered a side effect on pagerank.py ???

Since the linter says unused "type: ignore" comment, I'll try removing it...

Ok. Now mypy passes, but a ton of mypy data tests are now failing...
https://github.com/FurcyPin/spark/runs/6172104928?check_suite_focus=true

I really don't understand how this can be linked to my changes... and yet, the build on the branch master seems to work fine.

Guess I'll have to make another branch an push changes one by one until I find what's really happening...

HyukjinKwon · 2022-04-27T01:08:42Z

Yeah, I don't quite follow it either. Maybe let's fix it together in this PR, merge it and see how it goes.

## Update of previous commits Fix linter's warnings For some obscure reason, the CI linting step fails with the following error: starting mypy examples test... examples failed mypy checks: examples/src/main/python/pagerank.py:79: error: unused "type: ignore" comment This is extremely weird as the pagerank.py seems to be completely unrelated to the changes made in this ticket. We removed this "type: ignore" comment to make mypy happy.

HyukjinKwon · 2022-04-28T10:55:19Z

Merged to master.

github-actions Bot added CORE PYTHON SQL labels Apr 12, 2022

HyukjinKwon reviewed Apr 13, 2022

View reviewed changes

Comment thread python/pyspark/sql/session.py Outdated

HyukjinKwon reviewed Apr 14, 2022

View reviewed changes

Comment thread python/pyspark/sql/session.py Outdated

HyukjinKwon reviewed Apr 14, 2022

View reviewed changes

Comment thread python/pyspark/sql/session.py Outdated

viirya reviewed Apr 14, 2022

View reviewed changes

Comment thread python/pyspark/sql/session.py Outdated

viirya Apr 14, 2022

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

👍

viirya reviewed Apr 14, 2022

View reviewed changes

FurcyPin force-pushed the FurcyPin/SPARK-38870 branch from c1f43b9 to d696922 Compare April 14, 2022 07:29

dongjoon-hyun reviewed Apr 14, 2022

View reviewed changes

Comment thread python/pyspark/sql/session.py Outdated

dongjoon-hyun Apr 14, 2022

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

ditto.

dongjoon-hyun approved these changes Apr 14, 2022

View reviewed changes

HyukjinKwon approved these changes Apr 14, 2022

View reviewed changes

ueshin approved these changes Apr 14, 2022

View reviewed changes

BryanCutler approved these changes Apr 15, 2022

View reviewed changes

viirya approved these changes Apr 15, 2022

View reviewed changes

FurcyPin force-pushed the FurcyPin/SPARK-38870 branch from d696922 to 1ce3ad5 Compare April 15, 2022 07:51

HyukjinKwon reviewed Apr 18, 2022

View reviewed changes

FurcyPin force-pushed the FurcyPin/SPARK-38870 branch from 1ce3ad5 to 0c0bf73 Compare April 20, 2022 07:56

FurcyPin force-pushed the FurcyPin/SPARK-38870 branch from 0c0bf73 to 048cf00 Compare April 20, 2022 14:07

FurcyPin force-pushed the FurcyPin/SPARK-38870 branch 4 times, most recently from aa30677 to d882889 Compare April 22, 2022 08:48

Furcy Pin added 3 commits April 26, 2022 09:54

[SPARK-38870][SQL][PYSPARK] Make SparkSession.builder return new builder

8f5d5cd

## Update of previous commits Update docstring for SparkSession.builder, to make it match with Scala's description.

FurcyPin force-pushed the FurcyPin/SPARK-38870 branch from d882889 to 028298e Compare April 26, 2022 07:55

FurcyPin force-pushed the FurcyPin/SPARK-38870 branch from 028298e to a270630 Compare April 26, 2022 08:28

github-actions Bot added the EXAMPLES label Apr 26, 2022

FurcyPin force-pushed the FurcyPin/SPARK-38870 branch from a270630 to 79323e8 Compare April 28, 2022 08:44

HyukjinKwon closed this in 81e89cb Apr 28, 2022

FurcyPin deleted the FurcyPin/SPARK-38870 branch March 3, 2023 17:11

Uh oh!

Conversation

FurcyPin commented Apr 12, 2022

What changes were proposed in this pull request?

How was this patch tested?

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

AmplabJenkins commented Apr 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HyukjinKwon commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viirya Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

viirya left a comment

Choose a reason for hiding this comment

Uh oh!

FurcyPin commented Apr 14, 2022

Uh oh!

dongjoon-hyun Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

FurcyPin Apr 15, 2022

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Apr 14, 2022

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

ueshin left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Apr 15, 2022

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

FurcyPin commented Apr 15, 2022

Uh oh!

FurcyPin commented Apr 15, 2022

Uh oh!

HyukjinKwon commented Apr 18, 2022

Uh oh!

HyukjinKwon Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Apr 19, 2022

Uh oh!

FurcyPin commented Apr 19, 2022

Uh oh!

FurcyPin commented Apr 19, 2022

Uh oh!

HyukjinKwon commented Apr 19, 2022

Uh oh!

FurcyPin commented Apr 20, 2022

Uh oh!

FurcyPin commented Apr 20, 2022

Uh oh!

FurcyPin commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Apr 25, 2022

Uh oh!

FurcyPin commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Apr 27, 2022

Uh oh!

HyukjinKwon commented Apr 14, 2022 •

edited

Loading

FurcyPin commented Apr 22, 2022 •

edited

Loading

FurcyPin commented Apr 26, 2022 •

edited

Loading