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

Jump to bottom

[SPARK-40693][CONNECT] mypy complains accessing the variable defined in the class method. #38139

Closed
wants to merge 5 commits into from
Closed

[SPARK-40693][CONNECT] mypy complains accessing the variable defined in the class method. #38139

wants to merge 5 commits into from

Conversation

amaliujia
Copy link
Contributor

What changes were proposed in this pull request?

For SparkConnectTestsPlanOnly, those unit tests access the mock remote session by self.connect, how mypy complains in as error: "SparkConnectTestsPlanOnly" has no attribute "connect" [attr-defined]

Why are the changes needed?

Fix mypy.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Mypy.

@amaliujia
Copy link
Contributor Author

Another way to fix is to define the connect explicitly as

class SparkConnectSQLTestCase(ReusedPySparkTestCase):
    """Parent test fixture class for all Spark Connect related
    test cases."""

    connect: RemoteSparkSession
    ....

@amaliujia
Copy link
Contributor Author

Here is the build job: https://github.com/amaliujia/spark/actions/runs/3201556386

@amaliujia
Copy link
Contributor Author

R: @HyukjinKwon

@amaliujia amaliujia changed the title [SPARK-40693] mypy complains accessing the variable defined in the class method. [SPARK-40693][CONNECT] mypy complains accessing the variable defined in the class method. Oct 7, 2022
@HyukjinKwon
Copy link
Member

which mypy version do you use BTW? It's interesting that they all pass in CI.

@amaliujia
Copy link
Contributor Author

amaliujia commented Oct 7, 2022 •
edited

@HyukjinKwon

I just used ./dev/lint-python. I happened to add a new unit test here and my code was complained on the self.connect (as the same other tests) and mypy complains on my line of the code only. I am guessing mypy is triggered only for a new change somehow?

I don't think I have installed mypy by myself.

HyukjinKwon
HyukjinKwon reviewed Oct 7, 2022
View reviewed changes
python/pyspark/testing/connectutils.py Outdated
@@ -57,5 +57,7 @@ def __getattr__(self, item: str) -> Any:
class PlanOnlyTestFixture(unittest.TestCase):
@classmethod
def setUpClass(cls: Any) -> None:
cls.connect = MockRemoteSession()
cls._connect = MockRemoteSession()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cls._connect = MockRemoteSession()
cls.connect = MockRemoteSession()

HyukjinKwon
HyukjinKwon reviewed Oct 7, 2022
View reviewed changes
python/pyspark/testing/connectutils.py Outdated
cls.tbl_name = f"tbl{uuid.uuid4()}".replace("-", "")

connect: "MockRemoteSession"
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 put this type hint under class PlanOnlyTestFixture (see https://github.com/apache/spark/blob/master/python/pyspark/sql/tests/test_connect_basic.py#L34)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sure looks like we want to keep with the existing styple.

@amaliujia
Copy link
Contributor Author

amaliujia commented Oct 7, 2022 •
edited

The build: https://github.com/amaliujia/spark/actions/runs/3201742847

@HyukjinKwon
Copy link
Member

Merged to master.

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

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

Assignees
No one assigned
Labels
CORE PYTHON
Projects
None yet
Milestone
No milestone
2 participants
@amaliujia @HyukjinKwon