-
Notifications
You must be signed in to change notification settings - Fork 260
fix(spark): reject bool type as CPU in _validate_cpu_value #752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
962840c
e2d0d06
1ce0858
2001e33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -114,8 +114,8 @@ def _resolve_driver_resources( | |||||
| Tuple of (cores, memory). | ||||||
|
|
||||||
| Raises: | ||||||
| ValueError: | ||||||
| If the configured CPU or memory values are invalid. | ||||||
| ValueError: If the configured CPU or memory values are invalid. | ||||||
| TypeError: If an unsupported CPU type (such as bool) is passed. | ||||||
| """ | ||||||
|
|
||||||
| cores = constants.DEFAULT_DRIVER_CPU | ||||||
|
|
@@ -136,7 +136,7 @@ def _resolve_driver_resources( | |||||
| def _resolve_executor_resources( | ||||||
| executor: Executor | None = None, | ||||||
| num_executors: int | None = None, | ||||||
| resources_per_executor: dict[str, str] | None = None, | ||||||
| resources_per_executor: dict[str, str | int] | None = None, | ||||||
| ) -> tuple[int, int, str]: | ||||||
| """Resolve executor configuration. | ||||||
|
|
||||||
|
|
@@ -154,8 +154,8 @@ def _resolve_executor_resources( | |||||
| Tuple containing ``(instances, cores, memory)``. | ||||||
|
|
||||||
| Raises: | ||||||
| ValueError: | ||||||
| If the configured CPU or memory values are invalid. | ||||||
| ValueError: If the configured CPU or memory values are invalid. | ||||||
| TypeError: If an unsupported CPU type (such as bool) is passed. | ||||||
| """ | ||||||
|
|
||||||
| if executor and executor.num_instances is not None: | ||||||
|
|
@@ -257,11 +257,15 @@ def _validate_cpu_value(cpu: str | int | None) -> int: | |||||
| Integer CPU core value. | ||||||
|
|
||||||
| Raises: | ||||||
| ValueError: If CPU value is invalid. | ||||||
| ValueError: If CPU value is invalid or non-positive. | ||||||
| TypeError: If an unsupported type (such as bool) is passed. | ||||||
| """ | ||||||
| if cpu is None: | ||||||
| raise ValueError("CPU value cannot be None") | ||||||
|
|
||||||
| if isinstance(cpu, bool): | ||||||
| raise TypeError("Invalid CPU type 'bool'. Expected str or int.") | ||||||
|
|
||||||
| if isinstance(cpu, int): | ||||||
| cores = float(cpu) | ||||||
|
|
||||||
|
|
@@ -279,13 +283,19 @@ def _validate_cpu_value(cpu: str | int | None) -> int: | |||||
| f"Invalid CPU value '{cpu}'. Decimal milli-CPU values are not supported." | ||||||
| ) | ||||||
|
|
||||||
| cores = int(milli_cpu) / 1000 | ||||||
| try: | ||||||
| cores = int(milli_cpu) / 1000 | ||||||
| except ValueError as e: | ||||||
| raise ValueError(f"Invalid CPU value '{cpu}'.") from e | ||||||
|
|
||||||
| else: | ||||||
| cores = float(cpu) | ||||||
| try: | ||||||
| cores = float(cpu) | ||||||
| except ValueError as e: | ||||||
| raise ValueError(f"Invalid CPU value '{cpu}'.") from e | ||||||
|
|
||||||
| else: | ||||||
| raise ValueError(f"Invalid CPU type '{type(cpu)}'. Expected str or int.") | ||||||
| raise TypeError(f"Invalid CPU type '{type(cpu).__name__}'. Expected str or int.") | ||||||
|
|
||||||
| if not math.isfinite(cores) or cores <= 0: | ||||||
| raise ValueError(f"Invalid CPU value: {cpu!r}") | ||||||
|
|
@@ -473,7 +483,7 @@ def get_spark_connect_driver_spec( | |||||
| def get_spark_connect_executor_spec( | ||||||
| executor: Executor | None = None, | ||||||
| num_executors: int | None = None, | ||||||
| resources_per_executor: dict[str, str] | None = None, | ||||||
| resources_per_executor: dict[str, str | int] | None = None, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| ) -> models.SparkV1alpha1ExecutorSpec: | ||||||
| """Convert SDK Executor to API ExecutorSpec. | ||||||
|
|
||||||
|
|
@@ -490,8 +500,8 @@ def get_spark_connect_executor_spec( | |||||
| API ExecutorSpec model. | ||||||
|
|
||||||
| Raises: | ||||||
| ValueError: | ||||||
| If the configured executor resources are invalid. | ||||||
| ValueError: If the configured executor resources are invalid. | ||||||
| TypeError: If an unsupported CPU type (such as bool) is passed. | ||||||
| """ | ||||||
| instances, cores, memory = _resolve_executor_resources( | ||||||
| executor, | ||||||
|
|
@@ -511,7 +521,7 @@ def build_spark_connect_cr( | |||||
| namespace: str, | ||||||
| spark_version: str | None = None, | ||||||
| num_executors: int | None = None, | ||||||
| resources_per_executor: dict[str, str] | None = None, | ||||||
| resources_per_executor: dict[str, str | int] | None = None, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| spark_conf: dict[str, str] | None = None, | ||||||
| driver: Driver | None = None, | ||||||
| executor: Executor | None = None, | ||||||
|
|
@@ -542,8 +552,8 @@ def build_spark_connect_cr( | |||||
| SparkConnect CR as typed Pydantic model. | ||||||
|
|
||||||
| Raises: | ||||||
| ValueError: | ||||||
| If the provided driver or executor resource configuration is invalid. | ||||||
| ValueError: If the provided driver or executor resource configuration is invalid. | ||||||
| TypeError: If an unsupported CPU type (such as bool) is passed. | ||||||
| """ | ||||||
| _validate_spark_conf(spark_conf) | ||||||
|
|
||||||
|
|
@@ -688,7 +698,7 @@ def get_spark_job_driver_spec( | |||||
|
|
||||||
| def get_spark_job_executor_spec( | ||||||
| num_executors: int | None = None, | ||||||
| resources_per_executor: dict[str, str] | None = None, | ||||||
| resources_per_executor: dict[str, str | int] | None = None, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| ) -> models.SparkV1beta2ExecutorSpec: | ||||||
| """Build ExecutorSpec for SparkApplication. | ||||||
|
|
||||||
|
|
@@ -802,7 +812,7 @@ def get_spark_application_cr_from_file_job( | |||||
| main_file: str, | ||||||
| arguments: list[str] | None = None, | ||||||
| num_executors: int | None = None, | ||||||
| resources_per_executor: dict[str, str] | None = None, | ||||||
| resources_per_executor: dict[str, str | int] | None = None, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| options: list | None = None, | ||||||
| backend: Any | None = None, | ||||||
| spark_conf: dict[str, str] | None = None, | ||||||
|
|
@@ -865,7 +875,7 @@ def get_spark_application_cr_from_func_job( | |||||
| func: Callable, | ||||||
| func_args: dict[str, Any] | None = None, | ||||||
| num_executors: int | None = None, | ||||||
| resources_per_executor: dict[str, str] | None = None, | ||||||
| resources_per_executor: dict[str, str | int] | None = None, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| options: list | None = None, | ||||||
| backend: Any | None = None, | ||||||
| spark_conf: dict[str, str] | None = None, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -87,7 +87,7 @@ class Driver: | |||||||||||
| """ | ||||||||||||
|
|
||||||||||||
| image: str | None = None | ||||||||||||
| resources: dict[str, str] | None = None | ||||||||||||
| resources: dict[str, str | int] | None = None | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think best to follow the approach from other clients.
Suggested change
sdk/kubeflow/trainer/types/types.py Line 61 in 3febcd7
These should change as well. sdk/kubeflow/spark/api/spark_client.py Line 67 in 3febcd7
sdk/kubeflow/spark/api/spark_client.py Line 178 in 3febcd7
|
||||||||||||
| java_options: str | None = None | ||||||||||||
| service_account: str | None = None | ||||||||||||
|
|
||||||||||||
|
|
@@ -121,7 +121,7 @@ class Executor: | |||||||||||
| """ | ||||||||||||
|
|
||||||||||||
| num_instances: int | None = None | ||||||||||||
| resources_per_executor: dict[str, str] | None = None | ||||||||||||
| resources_per_executor: dict[str, str | int] | None = None | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here.
Suggested change
|
||||||||||||
| java_options: str | None = None | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.