Repository navigation
feat: hide ballista session contexts implementations behind common interface - #2832
Conversation
606842e to
9fddd65
Compare
9fddd65 to
9c80398
Compare
andygrove
left a comment
There was a problem hiding this comment.
Thanks @milenkovicm, this is a nice cleanup. One entry point that mirrors DataFusion's SessionContext is much easier to explain than two. I traced the dispatch for the native default, a passed client, ballista.python.client = "python", client plus "rust", and cluster_config with both clients, and it all looks right. The lazy import of .client keeps import ballista working without the extra too. A few small suggestions inline, none of them blocking.
|
|
||
| if client is None: | ||
| client = BallistaClient(address, config=config._ballista_overrides()) | ||
| return BallistaPythonSessionContext(client, config, runtime) |
There was a problem hiding this comment.
When someone passes client= and also puts ballista.client.pull or ballista.client.use_tls in config, those keys quietly don't apply to the client. The docs say so, but it's an easy trap. Would a UserWarning for ballista.client.* keys in that case be worth it? Happy to leave it documented if you prefer.
|
|
||
| # Python only option picking the cluster client, see `BallistaSessionContext`. | ||
| # It is kept out of `BallistaConfigExtension`, so it never reaches Rust. | ||
| PYTHON_CLIENT_KEY = "ballista.python.client" |
There was a problem hiding this comment.
Question on the key name. ballista.python.client sits in the ballista.* namespace but isn't a BallistaConfigExtension key. So SET ballista.python.client = 'python' fails with an unknown key error from Rust, and a future Rust key in the same namespace could clash. Also ballista.python.client = rust reads a little oddly. Have you considered something like ballista.python.client_impl? Or a short note in the docs that it can't be changed with SET? Not blocking either way.
|
|
||
| # client | ||
|
|
||
| def with_python_client(self, kind: str) -> "BallistaSessionConfig": |
There was a problem hiding this comment.
Nit: with_python_client("rust") reads a bit like it enables the Python client. Something like with_cluster_client("rust" | "python") might read more naturally. Totally optional.
| """ | ||
| return self.set(PYTHON_CLIENT_KEY, kind) | ||
|
|
||
| def _ballista_overrides(self) -> Dict[str, str]: |
There was a problem hiding this comment.
Nit: _ballista_overrides sits in the # client group of public with_* setters. Moving it next to build() would keep the setter list easier to scan.
| by default, they come with the `python-client` extra. | ||
| - A Ballista client implemented in Python, `BallistaClient`, is added. Pass | ||
| it to `BallistaSessionContext(client=...)`, or set `ballista.python.client` | ||
| to `python` in `config`, to use it instead of the native Rust client. Its dependencies are not installed by default, they come with |
There was a problem hiding this comment.
Nit: this bullet has one long line that wasn't rewrapped. Prettier is fine with it, so purely cosmetic.
|
thanks @andygrove i believe i have addressed all your comments in 0d64002 |
Which issue does this PR close?
Closes #.
Rationale for this change
The Python package has two entry points that do the same job:
BallistaSessionContext, which uses the native Rust client, andBallistaPythonSessionContext, which uses the PythonBallistaClient. Users have to pick one, even though the only real difference is which client talks to the cluster. Planning, configuration and the returneddatafusion.SessionContextare the same for both.We haven't decided yet which implementation to keep, so this PR keeps both and leaves a single entry point,
BallistaSessionContext. Aclientargument or the Python-only optionballista.python.clientpicks the client.Passing a
clientalone has a gap: the client is built outside the session, so theballista.*options inconfignever configure it (TLS, message size, keep-alive). Withballista.python.client = "python",BallistaSessionContextbuilds the client itself fromaddressandconfig.What changes are included in this PR?
BallistaSessionContextpicks the client as follows:client: BallistaClient, the Python client handles job submission, status tracking and Flight result fetching.addressis ignored, because the client has its own address.ballista.python.client = "python"inconfig, aBallistaClientis created foraddress, with theballista.*options set inconfig.ballista.python.client = "rust", it uses the native Rust client, same as before.clienttogether withballista.python.client = "rust"raisesValueError.ballista.python.clientis used only in Python:BallistaSessionConfig.set(and so a dict config) handles it before theballista.*key validation, and keeps it out ofBallistaConfigExtension. It never reaches Rust or the cluster, so there are no Rust changes.rustandpython, case-insensitive. Any other value raisesValueError.BallistaSessionConfig.with_python_client(kind).SessionConfig. In that case the native client is used.BallistaSessionContextis renamed toBallistaRustSessionContextinballista/extension.py. It andBallistaPythonSessionContextinballista/client.pyare now internal and are not exported fromballista.BallistaPythonSessionContextnow requiresclient. It no longer builds a default client itself;BallistaSessionContextdoes that whenballista.python.client = "python".BallistaSessionContexthandles the deprecatedcluster_configargument, so it works with both clients and the deprecation warning points at the caller.ballista.clientis imported only when the Python client is used, soimport ballistaand the native path still work without thepython-clientextra.ctxfixture intest_context.pyruns every context test against both clients throughBallistaSessionContext.test_client.pyuses the unified context.addressis ignored when a client is given;cluster_configworks together with a client.ballista.python.client:with_python_clientruns queries throughBallistaClient.ballista.*options reach the client (pull and push job tracking)."rust"use the native client.clienttogether with"rust"raises.examples/python_client.py;pyproject.tomlextra.Are there any user-facing changes?
Yes, in the Python API:
BallistaSessionContextselects the client and replacesBallistaPythonSessionContext(client=...):BallistaPythonSessionContextis no longer exported. It was added after 55.0.0 and only appears in the 56.0.0 upgrade guide, which hasn't been released, so no released API breaks. The 56.0.0 upgrade guide is updated to match.Option precedence with
ballista.python.client = "python":ballista.*options set inconfigbecome the client's own config, so they take precedence over laterSETstatements in that session.configcan still be changed withSET.BallistaClient(config=...).When a
clientis passed, theballista.*options inconfigdon't configure it. They still reach the scheduler with each query. Client-level options, such asballista.client.pull, go in theBallistaClientconfig.Existing
BallistaSessionContextcalls withoutclientorballista.python.clientbehave exactly as before.