Skip to content

chore: unify Python function validation between TrainerClient and SparkClient - #654

Open
mehulkumaryadav wants to merge 5 commits into
kubeflow:mainfrom
mehulkumaryadav:kep-107-typed-enums
Open

chore: unify Python function validation between TrainerClient and SparkClient#654
mehulkumaryadav wants to merge 5 commits into
kubeflow:mainfrom
mehulkumaryadav:kep-107-typed-enums

Conversation

@mehulkumaryadav

Copy link
Copy Markdown

Summary

Closes #637

This PR extracts the common Python function validation logic shared by TrainerClient and SparkClient into a reusable utility to ensure consistent validation behavior across both clients.

Changes

  • Added validate_python_function() to kubeflow.common.utils
  • Moved shared Python function validation logic from SparkClient into the common utility
  • Updated TrainerClient to use the shared validation utility
  • Kept Spark-specific validation (function arguments and heredoc delimiter checks) in SparkClient
  • Added unit tests for the shared validation utility
  • Updated Trainer utility and backend tests to use regular Python functions instead of lambda functions

Testing

  • Added unit tests for validate_python_function()
  • Updated Trainer utility tests
  • Updated Trainer backend tests
  • Ran project tests and pre-commit hooks

Signed-off-by: newtop7 <mehulop1415@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

🎉 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:

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

@google-oss-prow

Copy link
Copy Markdown
Contributor

[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 electronic-waste 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:

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

@lntutor lntutor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The decorated-function test does not exercise the decorator branch. @sample_decorator replaces sample_decorated_function with the nested wrapper; because that wrapper has no @... syntax, inspect.getsource(sample_decorated_function) yields the wrapper source and ast.FunctionDef.decorator_list is empty. The helper therefore accepts it instead of raising the asserted ValueError. Please use a pass-through decorator (def decorator(func): return func) for this test so the inspected function retains the decorated source, or otherwise test the actual callable/source pair that the validator receives.

Signed-off-by: newtop7 <mehulop1415@gmail.com>
@mehulkumaryadav

Copy link
Copy Markdown
Author

Thanks , I have addressed this feedback by updating the test to use a pass-through decorator so it exercises the actual decorator branch. The corrected version has been pushed.

@tariq-hasan

Copy link
Copy Markdown
Member

/ok-to-test
/retest

@tariq-hasan tariq-hasan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mehulkumaryadav! I have added a few comments.

Comment thread kubeflow/common/utils.py
)


def validate_python_function(func: Callable) -> None:

Copy link
Copy Markdown
Member

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.

Comment thread kubeflow/common/utils.py Outdated
Comment on lines +71 to +74
"""Validate that a Python function can be serialized and executed.

This validation is shared by TrainerClient and SparkClient.
"""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's match the Args, Raises, Returns docstring pattern from validate_wait_for_job_status.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the validation we can remove the duplicate logic in utils.py.

@@ -17,6 +17,27 @@
from kubeflow.trainer.test.common import SUCCESS, TestCase


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.
https://github.com/mehulkumaryadav/sdk/blob/2895b0780e39ece6e1ba9cc94af42f5ed2a99dbb/kubeflow/spark/backends/kubernetes/utils_test.py#L90

Signed-off-by: newtop7 <mehulop1415@gmail.com>
@mehulkumaryadav

Copy link
Copy Markdown
Author

@tariq-hasan @Goku2099 I have reviewed and addressed all the review comments. Please let me know if there's anything else you'd like me to update. Thanks!

Signed-off-by: newtop7 <mehulop1415@gmail.com>
Signed-off-by: newtop7 <mehulop1415@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unify function validation between TrainerClient and SparkClient

4 participants