-
Notifications
You must be signed in to change notification settings - Fork 225
chore: unify Python function validation between TrainerClient and SparkClient #654
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 all commits
9769e99
0a77abc
f6a5eda
12dbc46
89626dc
3981fe1
7f04de3
057f40e
51ceb2f
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 |
|---|---|---|
|
|
@@ -16,6 +16,36 @@ | |
| from kubeflow.common import utils | ||
| from kubeflow.trainer.test.common import SUCCESS, TestCase | ||
|
|
||
| # -------------------------- | ||
| # Test Helpers | ||
| # -------------------------- | ||
|
|
||
|
|
||
|
Contributor
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. it might be nice to keep using the same section-divider style (like # Test Helpers / # Tests above) across other test files too, just for consistency when browsing the suite. No strong opinion though. |
||
| def sample_function() -> None: | ||
| """Sample function for testing.""" | ||
| print("Hello World") | ||
|
|
||
|
|
||
| async def sample_async_function() -> None: | ||
| """Sample async function for testing.""" | ||
| pass | ||
|
|
||
|
|
||
| def sample_decorator(func): | ||
| """Pass-through decorator for testing.""" | ||
| return func | ||
|
|
||
|
|
||
| @sample_decorator | ||
| def sample_decorated_function() -> None: | ||
| """Sample decorated function.""" | ||
| pass | ||
|
|
||
|
|
||
| # -------------------------- | ||
| # Tests | ||
| # -------------------------- | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "test_case", | ||
|
|
@@ -67,3 +97,54 @@ def test_validate_wait_for_job_status(test_case): | |
| utils.validate_wait_for_job_status(polling_interval, timeout) | ||
| else: | ||
| utils.validate_wait_for_job_status(polling_interval, timeout) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "test_case", | ||
| [ | ||
| TestCase( | ||
| name="valid function", | ||
| expected_status=SUCCESS, | ||
| config={"func": sample_function}, | ||
| ), | ||
| TestCase( | ||
| name="not a function", | ||
| expected_status=SUCCESS, | ||
| config={"func": "not a function"}, | ||
| expected_error=ValueError, | ||
| expected_output="Function must be a Python function.", | ||
| ), | ||
| TestCase( | ||
| name="lambda function", | ||
| expected_status=SUCCESS, | ||
| config={"func": lambda: None}, | ||
| expected_error=ValueError, | ||
| expected_output="Lambda functions are not supported.", | ||
| ), | ||
| TestCase( | ||
| name="async function", | ||
| expected_status=SUCCESS, | ||
| config={"func": sample_async_function}, | ||
| expected_error=ValueError, | ||
| expected_output="Async functions are not supported.", | ||
| ), | ||
| TestCase( | ||
| name="decorated function", | ||
| expected_status=SUCCESS, | ||
| config={"func": sample_decorated_function}, | ||
| expected_error=ValueError, | ||
| expected_output="Decorated functions are not supported.", | ||
| ), | ||
| ], | ||
| ) | ||
| def test_validate_python_function(test_case): | ||
| """Test validate_python_function with valid and invalid functions.""" | ||
| print("Executing test:", test_case.name) | ||
|
|
||
| func = test_case.config["func"] | ||
|
|
||
| if test_case.expected_error: | ||
| with pytest.raises(test_case.expected_error, match=test_case.expected_output): | ||
| utils.validate_python_function(func) | ||
| else: | ||
| utils.validate_python_function(func) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,6 @@ | |
|
|
||
| """Kubernetes backend for Spark operations.""" | ||
|
|
||
| import ast | ||
| from collections.abc import Iterator | ||
| import contextlib | ||
| import inspect | ||
|
|
@@ -37,6 +36,7 @@ | |
|
|
||
| from kubeflow.common import constants as common_constants | ||
| from kubeflow.common.types import KubernetesBackendConfig | ||
| from kubeflow.common.utils import validate_python_function | ||
| from kubeflow.spark.backends.base import RuntimeBackend | ||
| from kubeflow.spark.backends.kubernetes import constants | ||
| from kubeflow.spark.backends.kubernetes.utils import ( | ||
|
|
@@ -853,29 +853,13 @@ def _validate_func_job( | |
| If the function or function arguments are invalid. | ||
| """ | ||
|
|
||
| if not inspect.isfunction(job.func): | ||
| raise ValueError("`job.func` must be a Python function.") | ||
| # Validate generic Python function properties. | ||
| validate_python_function(job.func) | ||
|
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. Given the validation we can remove the duplicate logic in |
||
|
|
||
| if inspect.iscoroutinefunction(job.func): | ||
| raise ValueError("Async functions are not supported.") | ||
|
|
||
| if job.func.__name__ == "<lambda>": | ||
| raise ValueError("Lambda functions are not supported.") | ||
|
|
||
| try: | ||
| func_source = textwrap.dedent(inspect.getsource(job.func)) | ||
| except TypeError as e: | ||
| raise ValueError( | ||
| "`job.func` must be a pure-Python function; built-in or " | ||
| "C-implemented callables are not supported." | ||
| ) from e | ||
| except OSError as e: | ||
| raise ValueError( | ||
| "`job.func` source could not be read. Functions defined " | ||
| "interactively (REPL/Jupyter) or generated dynamically are " | ||
| "not supported; define it in a Python module." | ||
| ) from e | ||
| # Get the function source for Spark-specific validation. | ||
| func_source = textwrap.dedent(inspect.getsource(job.func)) | ||
|
|
||
| # Ensure the function source does not contain the reserved heredoc delimiter. | ||
| if any( | ||
| line.strip() == constants.FUNC_JOB_SCRIPT_DELIMITER for line in func_source.splitlines() | ||
| ): | ||
|
|
@@ -884,17 +868,6 @@ def _validate_func_job( | |
| f"{constants.FUNC_JOB_SCRIPT_DELIMITER!r}, which is not supported." | ||
| ) | ||
|
|
||
| try: | ||
| func_tree = ast.parse(func_source) | ||
| except SyntaxError as e: | ||
| raise ValueError("`job.func` source could not be parsed.") from e | ||
|
|
||
| if any( | ||
| isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.decorator_list | ||
| for node in func_tree.body | ||
| ): | ||
| raise ValueError("Decorated functions are not supported.") | ||
|
|
||
| if job.func_args is not None: | ||
| if not isinstance(job.func_args, dict): | ||
| raise ValueError("`job.func_args` must be a dictionary.") | ||
|
|
||
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.
I think we want to apply this check to localprocess and container backend as well.