Skip to content

fix(spark): replace mutable default set in wait_for_job_status - #887

Open
SAURABHSALVE wants to merge 1 commit into
kubeflow:mainfrom
SAURABHSALVE:fix/spark-wait-for-job-status-mutable-default
Open

SAURABHSALVE wants to merge 1 commit into
kubeflow:mainfrom
SAURABHSALVE:fix/spark-wait-for-job-status-mutable-default

Conversation

@SAURABHSALVE

Copy link
Copy Markdown

What this PR does / why we need it:

wait_for_job_status in the Spark module still uses a mutable set literal as its status default, in all three places that define it: kubeflow/spark/api/spark_client.py, kubeflow/spark/backends/base.py, and kubeflow/spark/backends/kubernetes/backend.py.

status: set[SparkJobStatus] = {SparkJobStatus.COMPLETED},  # evaluated once, shared forever

This is the same anti-pattern reported in #329, but #330 and #393 (both currently open) only touch the Trainer and Optimizer modules — neither diff includes a single Spark file. #874 ("validate requested job statuses") edits these same three Spark files but only adds validation inside the function body; it doesn't touch the default-argument line either, so the mutable default survives that change too. This PR closes that gap for Spark.

Repro (see #885 for the full write-up): the default set object is created once and shared by every call that omits status. If any code mutates it in place (e.g. extends it with status.add(...) instead of copying first), every later call made without an explicit status argument silently picks up the mutated set for the rest of the process's lifetime.

Fix: default to None in all three signatures, and build a fresh {SparkJobStatus.COMPLETED} set inside spark_client.py and backends/kubernetes/backend.py when status is None — matching the fix pattern already used in #330/#393 for the other modules.

Verification

  1. Added three regression tests: two in spark_client_test.py (signature-default check, and a behavioral test that mutates the set returned to one call and asserts the next default-using call is unaffected) and one in backend_test.py (signature-default check).
  2. Confirmed all three new tests fail on unmodified main for the right reason (status.default is {SparkJobStatus.COMPLETED} instead of None; the mutation leaks into the second call).
  3. All three pass on this branch.
  4. Full targeted run: pytest kubeflow/spark/api/spark_client_test.py kubeflow/spark/backends/kubernetes/backend_test.py → 127 passed.
  5. ruff check, ruff format --check, and ty check kubeflow/spark on the changed files show no new issues (the 60 pre-existing ty diagnostics in this module are all in unrelated files/lines and present on main too).

Which issue(s) this PR fixes:

Fixes #885

Checklist:

  • No user-facing docs changes needed — this only changes an internal default-argument implementation detail; the documented default behavior ({SparkJobStatus.COMPLETED}) is unchanged.

🤖 Generated with Claude Code

status defaulted to a {SparkJobStatus.COMPLETED} set literal in
spark_client.py, backends/base.py, and backends/kubernetes/backend.py.
Python evaluates that literal once at function-definition time and
shares the same set object across every call that omits status, so an
in-place mutation of it (e.g. status.add(...)) would silently corrupt
the default target state for every later call, for the life of the
process.

Same anti-pattern as kubeflow#329, but kubeflow#330 and kubeflow#393 (both open) only touch
the Trainer and Optimizer modules, and kubeflow#874 only adds validation
inside the Spark wait_for_job_status body without touching the
default-argument line. None of them cover Spark.

Default to None and build a fresh {SparkJobStatus.COMPLETED} set
inside the function, matching the fix already used in kubeflow#330/kubeflow#393 for
the other modules.

Fixes kubeflow#885

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Saurabh Vijay Salve <saurabhsalve9999@gmail.com>
@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 astefanutti 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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Ready to Merge

Development

Successfully merging this pull request may close these issues.

bug(spark): wait_for_job_status still uses a mutable default set, not covered by #330/#393/#874

1 participant