fix: mutable default set argument in wait_for_job_status (#329)#393
fix: mutable default set argument in wait_for_job_status (#329)#393Adithya-S-01 wants to merge 1 commit intokubeflow:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
🎉 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:
Join the community:
Feel free to ask questions in the comments if you need any help or clarification! |
There was a problem hiding this comment.
Pull request overview
This PR removes mutable default set arguments from wait_for_job_status APIs across Trainer and Optimizer backends/clients, replacing them with None defaults and initializing to the appropriate “complete” status inside implementations.
Changes:
- Update
wait_for_job_statussignatures to usestatus: set[str] | None = Noneinstead of a mutable default set. - Initialize default status sets inside backend implementations (
TRAINJOB_COMPLETE/OPTIMIZATION_JOB_COMPLETE). - Remove now-unneeded
constantsimports where they were only used for the previous default argument.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| kubeflow/trainer/backends/localprocess/backend.py | Avoid mutable default by defaulting status to None and initializing to TRAINJOB_COMPLETE. |
| kubeflow/trainer/backends/kubernetes/backend.py | Same change for Kubernetes trainer backend, preserving validation and behavior. |
| kubeflow/trainer/backends/container/backend.py | Same change for container trainer backend, preserving behavior. |
| kubeflow/trainer/backends/base.py | Update abstract backend API signature to accept status=None. |
| kubeflow/trainer/api/trainer_client.py | Update client API signature to accept status=None and forward it. |
| kubeflow/optimizer/backends/kubernetes/backend.py | Avoid mutable default by initializing status to OPTIMIZATION_JOB_COMPLETE when None. |
| kubeflow/optimizer/backends/base.py | Update abstract optimizer backend API signature and remove unused constants import. |
| kubeflow/optimizer/api/optimizer_client.py | Update client API signature and remove unused constants import. |
Signed-off-by: Adithya <unknown0101200111@gmail.com>
3bb31f5 to
44f10f3
Compare
Fixes #329
Description
This PR resolves the mutable default argument anti-pattern present in the wait_for_job_status methods across the Trainer and Optimizer APIs. Previously, these methods defined
status: set[str] = {constants.TRAINJOB_COMPLETE}, which initializes the set object exactly once when the module is evaluated by the Python interpreter.Because sets are mutable, this pattern causes the exact same set instance to be shared across all subsequent invocations of wait_for_job_status within the same process. If any subclass or client code were to mutate this argument during execution, it would cause side effects across parallel or future SDK calls, leading to unpredictable job polling behavior.
status: set[str] | None = None.if status is None: status = {constants.*}to guarantee a fresh, isolated set per invocation.kubeflow.optimizer.constants.constantsimports from optimizer_client.py and optimizer/backends/base.py that were orphaned by removing the constants from the method signatures.make test-pythonsuite to ensure test coverage, alongsideuv run ruff checkanduv run ty checkinsidemake verifyfor strict type and format compliance.