Skip to content

Harden batch collections: cancellation, reattach, and error reporting - #317

Open
JyotinderSingh wants to merge 1 commit into
keras-team:mainfrom
JyotinderSingh:harden-batch-collections
Open

Harden batch collections: cancellation, reattach, and error reporting#317
JyotinderSingh wants to merge 1 commit into
keras-team:mainfrom
JyotinderSingh:harden-batch-collections

Conversation

@JyotinderSingh

Copy link
Copy Markdown
Collaborator

Description

Fixes four defects in kinetic.collections

  • cancel() now stops the whole collection. It sets a flag the submission loop reads, so inputs queued behind a bounded max_concurrent are dropped instead of launched, and cancelled indices are marked not-retryable so retries > 0 no longer resubmits a cancelled job when cancellation turns its status NOT_FOUND. A job registered while cancel() was mid-snapshot is cancelled by the submission loop, which closes that race.

  • attach_batch() decides completeness from the child indices the manifest names, not from how many handles loaded. results(cleanup=True) deletes a child's whole GCS prefix including handle.json, so a later reattach could not rebuild those children and blocked wait()/results() forever behind a poll thread with no timeout. A named child whose handle is gone is now terminal and reported by the new BatchHandle.unavailable_children, and poll_timeout defaults to 30 minutes. _all_accounted_for follows the same rule: once submission is complete the jobs list is frozen, so as_completed() no longer waits on slots that can never hold a job.

  • BatchError.failures holds only JobHandles, so the documented "for job in e.failures: job.job_id" no longer raises AttributeError. Inputs that fail at submission time are reported through the new BatchError.submission_failures, and they raise BatchError rather than being reachable only from the handle.

  • map(max_concurrent=None, retries=0, fail_fast=True) hands back a handle immediately again. Once every input is launched, fail_fast alone leaves the loop nothing to act on, so it no longer polls, and the calling-thread decision mirrors that predicate.

Contributor Agreement

  • I am a human, and not a bot.
  • I will be responsible for responding to review comments in a timely manner.
  • I will work with the maintainers to push this PR forward until submission.
  • I will test the changes on my cloud setup and provide proof of successful validation.

Four defects in kinetic.collections surfaced by a docs audit.

cancel() now stops the whole collection. It sets a flag the submission
loop reads, so inputs queued behind a bounded max_concurrent are dropped
instead of launched, and cancelled indices are marked not-retryable so
retries > 0 no longer resubmits a cancelled job when cancellation turns
its status NOT_FOUND. A job registered while cancel() was mid-snapshot is
cancelled by the submission loop, which closes that race.

attach_batch() decides completeness from the child indices the manifest
names, not from how many handles loaded. results(cleanup=True) deletes a
child's whole GCS prefix including handle.json, so a later reattach could
not rebuild those children and blocked wait()/results() forever behind a
poll thread with no timeout. A named child whose handle is gone is now
terminal and reported by the new BatchHandle.unavailable_children, and
poll_timeout defaults to 30 minutes. _all_accounted_for follows the same
rule: once submission is complete the jobs list is frozen, so
as_completed() no longer waits on slots that can never hold a job.

BatchError.failures holds only JobHandles, so the documented
"for job in e.failures: job.job_id" no longer raises AttributeError.
Inputs that fail at submission time are reported through the new
BatchError.submission_failures, and they raise BatchError rather than
being reachable only from the handle.

map(max_concurrent=None, retries=0, fail_fast=True) hands back a handle
immediately again. Once every input is launched, fail_fast alone leaves
the loop nothing to act on, so it no longer polls, and the
calling-thread decision mirrors that predicate.

Adds 23 tests across the four areas, each verified to fail against the
previous behaviour, and updates the batched-jobs guide and API reference.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 22.70115% with 269 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@a6adb41). Learn more about missing BASE report.

Files with missing lines Patch % Lines
kinetic/collections_test.py 0.00% 267 Missing ⚠️
kinetic/collections.py 97.53% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #317   +/-   ##
=======================================
  Coverage        ?   36.38%           
=======================================
  Files           ?       83           
  Lines           ?    15511           
  Branches        ?        0           
=======================================
  Hits            ?     5644           
  Misses          ?     9867           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request enhances the robustness of batched job execution, cancellation, and reattachment in Kinetic. Key changes include separating submission-time errors from runtime failures via a new submission_failures attribute, ensuring failures only contains valid JobHandles, and improving cancel() to drop queued inputs and prevent retries of cancelled jobs. Additionally, attach_batch() now handles cleaned-up children through unavailable_children and introduces a default 30-minute timeout for polling partially submitted manifests to prevent infinite blocking. Comprehensive unit tests have been added to cover these scenarios. I have no further feedback to provide as the implementation is robust, well-tested, and adheres to the repository's design guidelines.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants