[SPARK-46293][CONNECT][PYTHON] Use protobuf transitive dependency
#44221
Conversation
…ncy for Spark Connect
itholic
changed the title
protobuf to required dependency for Spark Connect
| @@ -161,6 +161,7 @@ Package Supported version Note | |||
| `numpy` >=1.21 Required for pandas API on Spark and MLLib DataFrame-based API; Optional for Spark SQL | |||
| `grpcio` >=1.59.3 Required for Spark Connect | |||
| `grpcio-status` >=1.59.3 Required for Spark Connect | |||
| `protobuf` ==4.25.1 Required for Spark Connect | |||
There was a problem hiding this comment.
The required version referred to
Line 56 in e013c4e
| @@ -161,6 +161,7 @@ Package Supported version Note | |||
| `numpy` >=1.21 Required for pandas API on Spark and MLLib DataFrame-based API; Optional for Spark SQL | |||
| `grpcio` >=1.59.3 Required for Spark Connect | |||
| `grpcio-status` >=1.59.3 Required for Spark Connect | |||
| `protobuf` ==4.25.1 Required for Spark Connect | |||
There was a problem hiding this comment.
you should fix https://github.com/apache/spark/blob/master/python/setup.py#L325C13-L331 too. in fact I won't add it for now because grpcio and grpcio-status requires a specific version of protobuf, and I think we won't have to duplicate the maintenance here. It is true that we directly use protobuf in few places but not really a lot of places.
There was a problem hiding this comment.
I see. Then what about just removing the protobuf from dev/requirements.txt as well to avoid confusion?? As you mentioned seems like grpcio and grpcio-status automatically detect the specific version and install it when even it's not specified in dev/requirements.txt as below:
There was a problem hiding this comment.
Otherwise we can close this ticket, but just one thing I'm worry about is that if the new version of grpcio, grpcio-status requires a protobuf version other than 4.25.1, I think pinning the specific version could be a problem.
There was a problem hiding this comment.
Can we check if we can remove protobuf dependency, and CI passes fine? if that works, let's remove.
If that doesn't work, we should fix the doc here and setup.py.
There was a problem hiding this comment.
Sure! Sounds reasonable to me.
itholic
changed the title
protobuf to required dependency for Spark Connectprotobuf to required dependency for Spark Connect
|
Let me mark this PR as draft until CI is done. |
itholic
changed the title
protobuf to required dependency for Spark Connectprotobuf from required package.
|
Okay, seems like we can just remove the dependency. Just updated the PR title & description. @HyukjinKwon FYI |
itholic
changed the title
protobuf from required package.protobuf from required package.
dongjoon-hyun
changed the title
protobuf from required package.protobuf from required package.
dongjoon-hyun
changed the title
protobuf from required package.protobuf transitive dependency
|
To @itholic and @HyukjinKwon , I revised the PR title because the original PR title is misleading. |
|
Merged to master for Apache Spark 4. |
|
Makes sense to me. Thanks for revising! |
### What changes were proposed in this pull request? This PR proposes to remove `protobuf` from required package. ### Why are the changes needed? `protobuf` is automatically installed when installing `grpcio` and `grpcio-status`, so we don't need to specify the specific version explicitly. ### Does this PR introduce _any_ user-facing change? No API changes. ### How was this patch tested? The existing CI should pass ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#44221 from itholic/protobuf_docs. Authored-by: Haejoon Lee <haejoon.lee@databricks.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
What changes were proposed in this pull request?
This PR proposes to remove
protobuffrom required package.Why are the changes needed?
protobufis automatically installed when installinggrpcioandgrpcio-status, so we don't need to specify the specific version explicitly.Does this PR introduce any user-facing change?
No API changes.
How was this patch tested?
The existing CI should pass
Was this patch authored or co-authored using generative AI tooling?
No.