Add Spark Connect Tests - CI & Test Suite Update #244

nijanthanvijayakumar · 2024-07-15T02:45:10Z

Proposed changes

This PR is regarding the open issue #241 to add a Spark-Connect test to the CI workflows and introduces the following changes:

Updates to existing files

Updates to the ci.yml file:
- Creates a new step under the test job to test a file using Spark-Connect server's SparkSession instance.
- Passes the Spark and Hadoop versions as environment variables.
- Uses a conditional SPARK_CONNECT_MODE_ENABLE to switch between the Spark-Connect session and the Local Spark session.
Updates the spark.py to create remote Spark-Connect instance:
- Uses a conditional to check if the environment configuration set at the CI is true or not to decide which Spark instance to be created/returned.
Updates the pyproject.toml:
- Adds the libraries: pandas = "^1.5.3", pyarrow = "16.1.0" numpy = "^1.21.0" grpcio = "^1.48.1" grpcio-status = "^1.64.1"
Updates the Makefile:
- Adds a line to skip the test_spark_connect.py from being PyTested using the regular tests.
- Adds a section to test_spark_connect.py separately.

New files:

Creates a new test_spark_connect.py file:
- This file contains a very basic test of one of the existing Quinn functions.
Creates a new shell script for running Spark-Connect server:
- Downloads the Spark binaries, installs and starts the Spark-Connect server.

Types of changes

New feature (non-breaking change which adds functionality)
Bugfix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices apply)

Further comments

This PR closes the issue: feat: Add spark-connect tests suite and run existing tests with SC #241
Please kindly review the versions of the dependencies used (pandas, pyarrow) and let me know if those versions are correct?
I have left a TODO comment in the run_spark_connect_server.sh to check if the spark-connect server is running or not. I tried netstat, which didn't work. I will work on that using a different PR.
Also, please let me know if the pr.yml file should also be updated for this Spark-Connect test case?

This commit introduces the following changes: * Updates the `ci.yml` file by introducing a new step under the `test` job to perform tests using Spark-Connect. * Creates a shell-script that downloads & installs Spark binaries and then runs the Spark-Connect server. * Creates a pytest module/file that tests a very simple function on Spark-Connect. * Updates the Makefile to add a new step for the Spark-Connect tests.

…n a condition

…dressed later

SemyonSinchenko · 2024-07-15T07:25:55Z

@nijanthanvijayakumar JFYI:

SemyonSinchenko · 2024-07-15T07:26:38Z

nijanthanvijayakumar · 2024-07-15T07:28:36Z

Amazing thank you! @SemyonSinchenko .

Looks like they haven't locked down pandas and numpy versions though. Are we okay with that?

pyproject.toml

SemyonSinchenko · 2024-07-15T07:32:25Z

Are we okay with that?

This dependencies should be optional for us (or be in the "provided" scope). Quinn itself should depends of python itself only.

Answering the question: we are mostly OK with that except numpy. The release of 2.0 was after the release of spark and Im more than sure that spark won't work with 2.0; so, I would like to add numpy < 2

nijanthanvijayakumar · 2024-07-15T07:34:51Z

Understood; thanks! This is an interesting PR/issue. I have been testing this spark-connect on the other test cases as well and have noted which ones might need to be worked on (for a later PR). Thanks for your guidance @SemyonSinchenko .

As per the review comment, the recently added dependencies such as Pyarrow, Pandas etc., are optional and not required for Spark-Classic. Update the pyproject.toml to reflect that and lock the poetry file

.github/workflows/ci.yml

pyproject.toml

…e deps

.github/workflows/ci.yml

… planning-1.0-release

nijanthanvijayakumar · 2024-07-15T08:58:07Z

Most likely fixes issue #247 as well. Please review and let me know.

SemyonSinchenko

I left some comments

tests/spark.py

scripts/run_spark_connect_server.sh

tests/test_spark_connect.py

Makefile

.github/workflows/ci.yml

nijanthanvijayakumar

Noted on the changes. Have responded to a few comments for clarifications.

* Remove the test_spark_connect.py file as it is not relevant anymore. * Update the Makefile to remove spark-connect test * Hardcode the hadoop version to 3 as 2 is EOL.

…connect-tests

nijanthanvijayakumar · 2024-07-15T11:08:00Z

@SemyonSinchenko - final go I reckon 🤞🏽 I have made all the changes. Happy for those comments to be resolved and the PR to be merged IF you are happy with it and if the tests pass.

SemyonSinchenko

LFTM

SemyonSinchenko · 2024-07-15T11:10:07Z

@nijanthanvijayakumar Thank you for the work!

nijanthanvijayakumar · 2024-07-15T11:11:19Z

@nijanthanvijayakumar Thank you for the work!

My pleasure. Thanks for the guidance and support.

nijanthanvijayakumar added 11 commits July 15, 2024 08:57

Address the fixtures issue in the test file

e1f0487

Update the CI workflow to initiate the sparkconnect test on the 1.0

c196228

Update the poetry & pyproject with the dependencies for Spark-Connect

1e4a439

Update the CI workflow to run Spark-Connect tests only for v3.4+

5cdc03b

Update the script to check if Spark-Connect server is running or not

37c271b

Remove the spark-connect server run check

b5f6749

Update workflows & pytest to choose the Sparksession instance based o…

6cdd1d2

…n a condition

Add a TODO statement so that the spark-connect server check can be ad…

3ed7ee1

…dressed later

Remove the 1.0 planning branch for the CI file

8879d59

Attribute the original script that inspired this

92ced5e

SemyonSinchenko reviewed Jul 15, 2024

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

Mark recently added deps as optional for Spark-Classic

1d8a0de

As per the review comment, the recently added dependencies such as Pyarrow, Pandas etc., are optional and not required for Spark-Classic. Update the pyproject.toml to reflect that and lock the poetry file

nijanthanvijayakumar commented Jul 15, 2024

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

SemyonSinchenko reviewed Jul 15, 2024

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

Rename the spark-classic to connect & update makefile to install thes…

b44a025

…e deps

nijanthanvijayakumar commented Jul 15, 2024

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

nijanthanvijayakumar added 2 commits July 15, 2024 18:43

Resolve the incoming commit with the makefile and import changes from…

5cf63f1

… planning-1.0-release

Fix the linting issues in the linting CI workflow

2f2f1a8

nijanthanvijayakumar requested a review from SemyonSinchenko July 15, 2024 08:58

SemyonSinchenko reviewed Jul 15, 2024

View reviewed changes

SemyonSinchenko mentioned this pull request Jul 15, 2024

Align ruff versions and apply hotfix #251

Merged

4 tasks

fpgmaas mentioned this pull request Jul 15, 2024

Improve pre-commit hooks and use of ruff #248

Merged

nijanthanvijayakumar commented Jul 15, 2024

View reviewed changes

nijanthanvijayakumar added 2 commits July 15, 2024 21:05

Update the files according to the review comments

4b829b6

* Remove the test_spark_connect.py file as it is not relevant anymore. * Update the Makefile to remove spark-connect test * Hardcode the hadoop version to 3 as 2 is EOL.

Merge branch 'planning-1.0-release' into feature/issue-241-add-spark-…

0da1804

…connect-tests

SemyonSinchenko approved these changes Jul 15, 2024

View reviewed changes

SemyonSinchenko merged commit cfcb720 into mrpowers-io:planning-1.0-release Jul 15, 2024
4 checks passed

nijanthanvijayakumar deleted the feature/issue-241-add-spark-connect-tests branch July 15, 2024 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Spark Connect Tests - CI & Test Suite Update #244

Add Spark Connect Tests - CI & Test Suite Update #244

nijanthanvijayakumar commented Jul 15, 2024 •

edited

Loading

SemyonSinchenko commented Jul 15, 2024

SemyonSinchenko commented Jul 15, 2024

nijanthanvijayakumar commented Jul 15, 2024

SemyonSinchenko commented Jul 15, 2024

nijanthanvijayakumar commented Jul 15, 2024

nijanthanvijayakumar commented Jul 15, 2024

SemyonSinchenko left a comment

nijanthanvijayakumar left a comment

nijanthanvijayakumar commented Jul 15, 2024 •

edited

Loading

SemyonSinchenko left a comment

SemyonSinchenko commented Jul 15, 2024

nijanthanvijayakumar commented Jul 15, 2024

Add Spark Connect Tests - CI & Test Suite Update #244

Add Spark Connect Tests - CI & Test Suite Update #244

Conversation

nijanthanvijayakumar commented Jul 15, 2024 • edited Loading

Proposed changes

Updates to existing files

New files:

Types of changes

Further comments

SemyonSinchenko commented Jul 15, 2024

SemyonSinchenko commented Jul 15, 2024

nijanthanvijayakumar commented Jul 15, 2024

SemyonSinchenko commented Jul 15, 2024

nijanthanvijayakumar commented Jul 15, 2024

nijanthanvijayakumar commented Jul 15, 2024

SemyonSinchenko left a comment

Choose a reason for hiding this comment

nijanthanvijayakumar left a comment

Choose a reason for hiding this comment

nijanthanvijayakumar commented Jul 15, 2024 • edited Loading

SemyonSinchenko left a comment

Choose a reason for hiding this comment

SemyonSinchenko commented Jul 15, 2024

nijanthanvijayakumar commented Jul 15, 2024

nijanthanvijayakumar commented Jul 15, 2024 •

edited

Loading

nijanthanvijayakumar commented Jul 15, 2024 •

edited

Loading