fix(spark): reject bool type as CPU in _validate_cpu_value

[Neha-Nimsatkar]

What this PR does / why we need it:

• Scopes CPU validation in _validate_cpu_value strictly to rejecting bool types (True/False), which previously passed as 1 core due to Python's isinstance(True, int) subclassing behavior.
• Removes float CPU support and reverts resource dictionary type hints to align with backend SparkV1alpha1ServerSpec core allocation semantics.
• Raises TypeError when an unsupported argument type (e.g. bool, list, dict) is passed.
• Updates unit tests in utils_test.py and types_test.py.

Which issue(s) this PR fixes:

N/A

Checklist:

• Pre-commit checks passed
• Unit tests passed
• DCO Signed-off

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kramaranya for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

🎉 Welcome to the Kubeflow SDK! 🎉

Thanks for opening your first PR! We're happy to have you as part of our community 🚀

Here's what happens next:

• If you haven't already, please check out our Contributing Guide for repo-specific guidelines and the Kubeflow Contributor Guide for general community standards
• Our team will review your PR soon! cc @kubeflow/kubeflow-sdk-team

Join the community:

• Slack: Join our #kubeflow-ml-experience and #kubeflow-trainer Slack channels
• Meetings: Attend the Kubeflow SDK and ML Experience bi-weekly meetings

Feel free to ask questions in the comments if you need any help or clarification!
Thanks again for contributing to Kubeflow! 🙏

[tariq-hasan]

/ok-to-test

[tariq-hasan]

Hi @Neha-Nimsatkar! Thanks for raising the PR.

You'd need to fix the errors in pre-commit to pass the CI.

[HarshPopat23]

I reviewed the latest head (d596f10). Rejecting bool is a valid correctness fix because bool subclasses int in Python, so the current implementation incorrectly accepts True as one CPU core.

I still see three points that should be addressed before merging:

Blocking 1: Public API types do not support the new float input

The private helper now accepts float, but the user-facing APIs still declare resource dictionaries as dict[str, str]. This includes SparkClient.connect(), SparkClient.submit_job(), Driver.resources, Executor.resources_per_executor, and the relevant resolver/builder methods.

As a result, resources_per_executor={"cpu": 1.5} works dynamically but is rejected by static type checkers and IDEs. Could we either update the public resource-value type annotations and documentation to make numeric CPU values an official part of the API, or keep the public contract string-based and scope this PR to the boolean-validation fix?

Blocking 2: Please clarify the float semantics

1.5 is normalized with math.ceil() and becomes 2. The generated CR therefore receives two Spark cores; it does not preserve a fractional 1.5-CPU Kubernetes request.

The Spark Operator distinguishes the integer cores field, which maps to spark.executor.cores, from the string coreRequest field used for physical Kubernetes CPU requests. The current title and description could therefore be interpreted as fractional CPU-request support even though this change only accepts a float representation and rounds it to an integer core count.

Could we document that fractional values are rounded upward? If the intention is to support fractional Kubernetes CPU allocation, this would need to target coreRequest instead.

Blocking 3: Propagate the new exception contract

Changing unsupported types from ValueError to TypeError is semantically reasonable, but that exception propagates through public methods. The resolver, spec-builder, and SparkClient.connect() docstrings still document only ValueError for invalid resource configuration.

Could we update the relevant Raises sections so users can reliably handle this new exception type?

Please also add at least one test through a public construction path, such as get_spark_connect_executor_spec(resources_per_executor={"cpu": 1.5}), and assert that the resulting CR field is cores == 2. A similar driver-resource test would cover the advanced API path.

The explicit boolean rejection itself looks correct.

[google-oss-prow]

@HarshPopat23: changing LGTM is restricted to collaborators

Details

In response to this:

I reviewed the latest head (d596f10). Rejecting bool is a valid correctness fix because bool subclasses int in Python, so the current implementation incorrectly accepts True as one CPU core.

I still see three points that should be addressed before merging:

Blocking 1: Public API types do not support the new float input

The private helper now accepts float, but the user-facing APIs still declare resource dictionaries as dict[str, str]. This includes SparkClient.connect(), SparkClient.submit_job(), Driver.resources, Executor.resources_per_executor, and the relevant resolver/builder methods.

As a result, resources_per_executor={"cpu": 1.5} works dynamically but is rejected by static type checkers and IDEs. Could we either update the public resource-value type annotations and documentation to make numeric CPU values an official part of the API, or keep the public contract string-based and scope this PR to the boolean-validation fix?

Blocking 2: Please clarify the float semantics

1.5 is normalized with math.ceil() and becomes 2. The generated CR therefore receives two Spark cores; it does not preserve a fractional 1.5-CPU Kubernetes request.

The Spark Operator distinguishes the integer cores field, which maps to spark.executor.cores, from the string coreRequest field used for physical Kubernetes CPU requests. The current title and description could therefore be interpreted as fractional CPU-request support even though this change only accepts a float representation and rounds it to an integer core count.

Could we document that fractional values are rounded upward? If the intention is to support fractional Kubernetes CPU allocation, this would need to target coreRequest instead.

Blocking 3: Propagate the new exception contract

Changing unsupported types from ValueError to TypeError is semantically reasonable, but that exception propagates through public methods. The resolver, spec-builder, and SparkClient.connect() docstrings still document only ValueError for invalid resource configuration.

Could we update the relevant Raises sections so users can reliably handle this new exception type?

Please also add at least one test through a public construction path, such as get_spark_connect_executor_spec(resources_per_executor={"cpu": 1.5}), and assert that the resulting CR field is cores == 2. A similar driver-resource test would cover the advanced API path.

The explicit boolean rejection itself looks correct.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Neha-Nimsatkar]

Thanks @HarshPopat23 for the thorough review!

I have updated the PR to address all three points:

1. Public API Types: Updated resources_per_executor type annotations to dict[str, str | int | float] across the relevant helper and builder functions.
2. Float Semantics: Documented in _validate_cpu_value that fractional CPU values are rounded upward (math.ceil) for Spark executor cores.
3. Exception Contract & Tests: Documented TypeError in the relevant Raises sections and added public construction path unit tests in utils_test.py (test_get_spark_connect_executor_spec_float_cpu and test_get_spark_connect_executor_spec_bool_cpu_raises_type_error).

All unit tests, Ruff linter checks, and pre-commit hooks are passing cleanly. Please let me know if any further tweaks are needed!

[Goku2099]

Non-blocking nit: dict[str, str | int | float] | None is now repeated across multiple signatures in this file. Might be worth pulling it into a shared alias in constants.py (e.g. ResourceDict = dict[str, str | int | float]) so future changes to accepted types only need one edit. Could be a fast-follow rather than blocking this PR.. @tariq-hasan thoughts?

[Neha-Nimsatkar]

Thanks @Goku2099! Extracting ResourceDict = dict[str, str | int | float] into constants.py makes total sense to clean up duplicate signatures across the file. I'll be happy to open a quick fast-follow PR for that right after this one is merged!

[Goku2099]

Thanks for the PR!
/lgtm
cc @tariq-hasan

[google-oss-prow]

New changes are detected. LGTM label has been removed.

[Neha-Nimsatkar]

/retest

[tariq-hasan]

@Neha-Nimsatkar @Goku2099 @HarshPopat23 Looking into the PR changes a bit more I think we should keep this PR focused only on rejecting bool type as CPU.

The reason is because the CR construction step currently converts resources into an integer number of cores and not directly CPU request and limit.

sdk/kubeflow/spark/backends/kubernetes/utils.py

Lines 466 to 470 in 3febcd7

	return models.SparkV1alpha1ServerSpec(
	cores=cores,
	memory=memory,
	template=template,
	)

There is an issue raised in the spark-operator project to support request and limit directly on the SparkConnect CRD - similar to SparkApplication- kubeflow/spark-operator#2957. Once this is completed we can revisit how to support non-integer resources directly through CPU request and limit on the SDK - to ensure the semantics of resources on the SDK matches the expectation that this be treated as K8s-native resource allocation instead of Spark-native interpretation of task parallelism.

@Neha-Nimsatkar Please remove the support for float CPU values for now from the PR so we can move forward with the changes.

[Neha-Nimsatkar]

@Neha-Nimsatkar @Goku2099 @HarshPopat23 Looking into the PR changes a bit more I think we should keep this PR focused only on rejecting bool type as CPU.

The reason is because the CR construction step currently converts resources into an integer number of cores and not directly CPU request and limit.

sdk/kubeflow/spark/backends/kubernetes/utils.py

Lines 466 to 470 in 3febcd7

	return models.SparkV1alpha1ServerSpec(
	cores=cores,
	memory=memory,
	template=template,
	)

There is an issue raised in the spark-operator project to support request and limit directly on the SparkConnect CRD - similar to SparkApplication- kubeflow/spark-operator#2957. Once this is completed we can revisit how to support non-integer resources directly through CPU request and limit on the SDK - to ensure the semantics of resources on the SDK matches the expectation that this be treated as K8s-native resource allocation instead of Spark-native interpretation of task parallelism.

@Neha-Nimsatkar Please remove the support for float CPU values for now from the PR so we can move forward with the changes.

Thanks for the feedback @tariq-hasan! That makes complete sense regarding the backend integer cores mapping and the Spark Operator CRD tracking.

I have updated the PR to remove float CPU support and scoped the changes strictly to rejecting bool types (along with updating the unit tests). All linting, pre-commit checks, and test suites are passing locally. Please take a look when you have a chance!

[Neha-Nimsatkar]

/test E2E Test (1.34.0, master)

[google-oss-prow]

@Neha-Nimsatkar: No presubmit jobs available for kubeflow/sdk@main

Details

In response to this:

/test E2E Test (1.34.0, master)

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Neha-Nimsatkar]

cc @tariq-hasan Hi Tariq, the E2E Test (1.34.0, master) job hit an infrastructure timeout ("self-hosted runner lost communication") while all other versions passed cleanly. Could you please click "Re-run failed jobs" when you get a chance? Thanks!

[Goku2099]

/ok-to-test
/retest

[tariq-hasan]

I think best to follow the approach from other clients.

Suggested change

	resources: dict[str, str | int] | None = None
	resources: dict | None = None

sdk/kubeflow/trainer/types/types.py

Line 61 in 3febcd7

resources_per_node: dict | None = None

These should change as well.

sdk/kubeflow/spark/api/spark_client.py

Line 67 in 3febcd7

resources_per_executor: dict[str, str] | None = None,

sdk/kubeflow/spark/api/spark_client.py

Line 178 in 3febcd7

resources_per_executor: dict[str, str] | None = None,

[tariq-hasan]

Same here.

Suggested change

	resources_per_executor: dict[str, str | int] | None = None
	resources_per_executor: dict | None = None

[tariq-hasan]

Suggested change

	resources_per_executor: dict[str, str | int] | None = None,
	resources_per_executor: dict | None = None,

[tariq-hasan]

Suggested change

	resources_per_executor: dict[str, str | int] | None = None,
	resources_per_executor: dict | None = None,

[tariq-hasan]

Suggested change

	resources_per_executor: dict[str, str | int] | None = None,
	resources_per_executor: dict | None = None,

[tariq-hasan]

Suggested change

	resources_per_executor: dict[str, str | int] | None = None,
	resources_per_executor: dict | None = None,

[tariq-hasan]

Suggested change

	resources_per_executor: dict[str, str | int] | None = None,
	resources_per_executor: dict | None = None,

[tariq-hasan]

Suggested change

	resources_per_executor: dict[str, str | int] | None = None,
	resources_per_executor: dict | None = None,

[Neha-Nimsatkar]

Thanks for the review and guidance, @tariq-hasan!

I have addressed all the feedback and pushed the updates:

• Simplified resources and resources_per_executor annotations to dict | None = None across types.py and utils.py to match the convention in kubeflow.trainer.
• Updated the two occurrences in spark_client.py (lines 67 and 178) as well.
• Ran pre-commit hooks and confirmed all Spark unit tests are passing locally.

Ready for another look whenever you have time!