[SPARK-58559][PYTHON] Package all of sbin in PySpark classic distribution

[nchammas]

What changes were proposed in this pull request?

Package everything in sbin/ when building a PySpark Classic distribution.

Rely on a clear project support policy -- clarified in #57452 -- rather than micro-managing what gets packaged to communicate to users that PySpark is not meant to launch "real" clusters.

Why are the changes needed?

Some sbin scripts were first added to PySpark in #23715. There was some disagreement then about whether PySpark should include these scripts, mainly because some committers at the time thought PySpark should be a client-only distribution.

In the intervening years, the Spark Connect effort has created a true client-only distribution of Spark in contrast to the "heavier" PySpark Classic which includes all of Spark's assembly JARs.

PySpark Classic has included some sbin scripts since 3.0.0, and #56907 recently added the Connect server scripts. #56907, however, neglected to add the corresponding directives to MANIFEST.in.

Since #57452 clarifies the intended use of the Python distributions of Spark -- specifically, that starting a full cluster is not supported, regardless of whether it's possible -- I believe it's conceptually simpler to just package all of sbin. That would, for example, prevent the kind of gap identified in #56907.

Does this PR introduce any user-facing change?

Yes, it packages additional sbin scripts in the PySpark classic distribution.

How was this patch tested?

Distributions are not tested thoroughly. #57645 adds a dedicated distribution validation script. I think we should discuss there any testing we would like to add for this.

Was this patch authored or co-authored using generative AI tooling?

No.

[nchammas]

cc @gaogaotiantian @HyukjinKwon @ericm-db

[HyukjinKwon]

I think the problem was that we wanted to avoid for users to use pip installed pyspark to start a cluster or sth. what scripts do we include?

[nchammas]

Yes, we wanted to prevent users from using PySpark to start a standalone cluster. I tried to address this with a clearer README so that we can just package everything. In other words, I think it's easier to control this via our support policy instead of via "hard" packaging changes.

Switching to * means we will package sbin/start-{thriftserver, master, worker}.sh in addition to the existing scripts we are already packaging today. These newly packaged scripts can also be called by the new CLI if/when we merge it.

[uros-b]

@nchammas Please note that switching from an explicit whitelist to ["*"] / graft deps/sbin adds 13 cluster-management scripts (start-all.sh, stop-all.sh, start-master.sh, stop-master.sh, start-workers.sh, stop-workers.sh, start-worker.sh, stop-worker.sh, start-thriftserver.sh, stop-thriftserver.sh, decommission-worker.sh, spark-daemons.sh, workers.sh) that were deliberately excluded in the original #23715 discussion due to concerns about users trying to start full clusters from a pip-installed package.

[nchammas]

Correct. Per the PR description and my comment just above, I am proposing we make it a clear project policy not to support launching clusters with PySpark vs. micro-managing what gets packaged. The latter is more annoying to maintain and leads to the packaging bug described in the PR description.

[gaogaotiantian]

I think an issue we need to think here is that it's difficult for us to unpackage the scripts. Once we decide to ship it, it would be a breaking change to no ship it anymore. So should we add this only when we want the users to have it?

[nchammas]

I think that's a reasonable approach, but in that case I'd want us to add a test or linter to ensure that setup.py and MANIFEST.in stay in sync, because that was one of my motivations for doing this (though not the only one).

If we just package everything, it's simpler to maintain. And I don't know how much of a breaking change it would be if we are clear upfront in the README -- as I've done in #57452 -- that launching a full cluster using PySpark is not supported.

[gaogaotiantian]

I think we have a pip test somewhere to test packaging? Yes it would be nice to make sure MANIFEST.in syncs with setup.py.

I'm more worried about future. In a few months this packaging detail might be forgotten (or never noticed). People may add new "dev-only" scripts to sbin and accidentally ship it. I'm not saying this is a huge deal breaker. I just like explicit allow list than wildcards.

[nchammas]

I'm more worried about future. In a few months this packaging detail might be forgotten (or never noticed). People may add new "dev-only" scripts to sbin and accidentally ship it. I'm not saying this is a huge deal breaker. I just like explicit allow list than wildcards.

I'm not so worried about this because we already package all of bin. Both bin and sbin are specifically for end-users; a dev-only script would go in dev.

Would it help if I added a very brief README to both bin and sbin making this explicit?

[gaogaotiantian]

Okay I believe a general rule is - if you can avoid giving users the wrong tools, you should not give them the tools and tell them not to use them. You won't give a child a knife and tell them - please don't use it, I'm not responsible for it.

Documentation is the last stop. Sure we can claim that we told the users not to use them, but that won't make the users happier. "I told you so" is a bad user experience. We are not lawyers.

If we have to ship something to users that they are not supposed to use, sure we can somehow put the words somewhere (that users may never get the chance to read it). But for anyone who has been working on open source projects for a while, it's an inevitable fact that any internal details will become a public binding after enough time.

So this is a tradeoff. We ship something users are not supposed to use, which is obviously bad. What do we gain from shipping them? Is it something we can get from another way? Is it important to us that it could save us a lot of time and trouble? Will it impact users in other ways?

To the specific issue you raised - it actually does not matter (that much). The file will be picked up in sdist if it appears in either MANIFEST.in or package_data. It's nice to keep consistency but it's probably not necessary. We also have a pip test to catch packaging issues. So I don't see a clear benefit from doing this.

[nchammas]

I agree with this general rule but don't see that it applies so strongly in this case. sbin scripts are for end-users. We already package them all in our "full" distributions of Spark. PySpark is the exception. (It's not even clear to me why we wouldn't support users launching "real" clusters with PySpark since the JARs are the same, but that's a separate issue.)

The file will be picked up in sdist if it appears in either MANIFEST.in or package_data.

I didn't realize this and just tested it to confirm. This is also documented:

Note that the data files specified using the package_data option neither require to be included within a MANIFEST.in file, nor require to be added by a revision control system plugin.

In this case I would want us to remove anything from MANIFEST.in that doesn't need to be there, because the duplicate entries across setup.py and MANIFEST.in are a source of confusion. I will submit a PR for that and set aside this proposal for now.

[gaogaotiantian]

I understand that the scripts are for end users, but if we don't believe users should use it through pyspark, we should not ship them. However, if we somehow decide that users that install pyspark should have access to those scripts as well, we can include the full directory. It might be the case.

For the current situation, as long as we think those should not be used by end users, we should not ship them.

As for MANIFEST.in, it has a complicated history with package_data I believe. We can take a look at it, but I think that file was shared between all the 3 packages(classic, client, connect) we have? There could be side effects.

[nchammas]

Closing in favor of #57973.