Skip to content

feat(job): create job definition only after successful upload#687

Merged
RapidPoseidon merged 2 commits into
mainfrom
feat(job)/atomic-job-definition-creation
Jul 21, 2026
Merged

feat(job): create job definition only after successful upload#687
RapidPoseidon merged 2 commits into
mainfrom
feat(job)/atomic-job-definition-creation

Conversation

@RapidPoseidon

Copy link
Copy Markdown
Contributor

Problem

Job-definition creation wasn't atomic. In _create_general_job_definition the flow was: create dataset → upload datapoints → persist the definition unconditionallythen raise FailedUploadException if anything failed. So a partial upload left behind an incomplete-but-existing definition. Worse, the natural retry — re-calling create_*_job_definition(...) — spun up a second dataset and definition, and the documented alternative (e.dataset.add_datapoints(e.failed_uploads)) dropped the caller into a lower-level API than the one they used to create the job. Reported from an SDK 3.16.5 Compare integration.

Change

Model creation as an explicit state machine (JobDefinitionCreationMachine): create dataset → upload datapoints → persist definition, reaching the persist step only once the upload lands within a configurable failure tolerance.

  • Atomic by default. On failure no definition is created — no more phantom incomplete definitions. (Mirrors what update_dataset already did for revisions.)
  • Smooth, symmetric recovery. The raised FailedUploadException carries the machine, so exception.retry() re-uploads only the failed datapoints into the same dataset and finishes creating the definition — returns a RapidataJobDefinition, re-raises if still failing. No duplicate dataset/definition; recovery speaks the same language as creation. dataset.add_datapoints stays as an advanced escape hatch.
  • failure_tolerance — fraction of a job's datapoints allowed to fail while still creating the definition (0.0 strict default … 1.0 create regardless). Global via rapidata_config.upload.failureTolerance / RAPIDATA_failureTolerance, or per-call via the new failure_tolerance argument on every create_*_job_definition.

Behavior change (intentional)

Previously a partial failure still yielded a runnable definition with the datapoints that succeeded. Now the default (failure_tolerance=0.0) yields no definition until you retry. Set failure_tolerance above 0.0 (or 1.0) to keep the old "proceed with what uploaded" behavior. Docs (error_handling.md) rewritten to lead with retry() + tolerance.

Known limitation

There's no dataset-delete endpoint, so a failed run still leaves an orphaned dataset (never an orphaned definition), and a retry reuses it. Fully removing the orphan needs backend support (idempotency key / dataset delete) — out of scope here.

Tests

New tests/rapidata_client/job/test_job_creation_state_machine.py: full-success creates once; failure within tolerance still creates; failure beyond tolerance raises with no definition; retry() reuses the same dataset and persists exactly one definition. Full suite green (11 passed), pyright clean, black clean, docs build.

🔗 Session: https://node-218fd5bc.poseidon.rapidata.internal/

🤖 Generated with Claude Code

Job-definition creation was not atomic: the dataset was created, datapoints
were uploaded, and the definition was persisted unconditionally - even when
some datapoints failed - then a FailedUploadException was raised afterwards.
A partial upload therefore left behind an incomplete-but-existing definition,
and the only documented retry (dataset.add_datapoints) both dropped into a
lower-level API than the one used to create the job and, if the caller instead
re-ran create_*_job_definition, spun up a second dataset and definition.

Model creation as an explicit state machine (JobDefinitionCreationMachine):
create dataset -> upload datapoints -> persist definition, reaching the persist
step only once the upload lands within a configurable failure tolerance. On
failure no definition is created; the raised FailedUploadException carries the
machine so exception.retry() re-uploads only the failed datapoints into the
SAME dataset and finishes creating the definition - no duplicate dataset or
definition, and recovery speaks the same language as creation.

failure_tolerance is the fraction of a job's datapoints allowed to fail while
still creating the definition (0.0 = strict default, 1.0 = create regardless).
Set it globally via rapidata_config.upload.failureTolerance /
RAPIDATA_failureTolerance, or per call via the new failure_tolerance argument.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: lino <68745352+LinoGiger@users.noreply.github.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Review: atomic job-definition creation via state machine

Solid design for the core problem (no more phantom incomplete definitions, symmetric retry() reusing the same dataset). One correctness bug in the tolerance logic, plus a couple of minor nits.

Bug: failure_tolerance=1.0 doesn't "always create" when all datapoints fail

In _job_creation_state_machine.py (_upload_datapoints):

within_tolerance = failure_ratio <= self._failure_tolerance
if within_tolerance and self._succeeded_count > 0:
    ...
    self._state = _State.PERSIST_DEFINITION
else:
    self._state = _State.FAILED

The extra self._succeeded_count > 0 guard means that when every datapoint fails, the definition is never persisted — regardless of failure_tolerance. But both the docstring on JobDefinitionCreationMachine and UploadConfig.failureTolerance, plus docs/error_handling.md, explicitly document 1.0 as "the definition is always created, regardless of how many datapoints failed."

So a caller who sets failure_tolerance=1.0 specifically to guarantee create_*_job_definition never raises will still see FailedUploadException raised in the (admittedly edge-case) scenario where the entire upload fails (e.g. a transient full outage). Worth either:

  • fixing the condition to only special-case tolerance 1.0 (or drop the succeeded_count > 0 guard and decide explicitly whether a 0-datapoint definition is acceptable), or
  • if a 0-datapoint definition is intentionally disallowed, documenting that exception to the "1.0 = regardless" rule in the docstrings/docs.

This edge case isn't covered by test_job_creation_state_machine.py (the tolerance tests only exercise partial failure), which is presumably why it slipped through — worth adding a test for "100% failure, tolerance=1.0" once the behavior is decided.

Minor nits

  • _create_general_job_definition re-validates 0.0 <= failure_tolerance <= 1.0 with its own ValueError, duplicating UploadConfig.validate_failure_tolerance's field_validator. Not wrong, just a second place to keep in sync if the bound ever changes.
  • In JobDefinitionCreationMachine.run(), the FAILED branch passes job_definition=self.job_definition to FailedUploadException, which is always None at that point (the only place job_definition is set transitions to SUCCEEDED, not FAILED). Harmless, but could just pass None explicitly for clarity.

What's good

  • The dataset/definition separation cleanly fixes the atomicity bug described in the PR — no definition until upload is within tolerance.
  • retry() correctly reuses the same dataset and only re-uploads previously-failed items; _succeeded_count accumulates across retries so the tolerance ratio stays meaningful (measured against the original total, per the docstring).
  • FailedUploadException.retry() raising RuntimeError when machine is None (order-path failures) is a good guard against misuse.
  • Docs rewrite in error_handling.md clearly leads with retry() and calls out the "don't re-call create_*_job_definition" pitfall.
  • Good test coverage for the main paths (full success, partial-within-tolerance, exceeds-tolerance, retry-reuses-dataset), including the tolerance boundary (<=) case.

No security or performance concerns — this only changes ordering/control-flow around existing upload calls.

@LinoGiger
LinoGiger marked this pull request as ready for review July 21, 2026 09:23
@LinoGiger
LinoGiger self-requested a review as a code owner July 21, 2026 09:23
Addresses review feedback: failure_tolerance=1.0 was documented as "always
create regardless of failures", but the at-least-one-success floor meant a
100%-failed upload still raised. That floor is intentional - a definition over
an empty dataset has nothing to label and would be exactly the useless remote
artifact this change prevents - so the behavior stays and the docstrings/docs
now state the caveat explicitly, with a test covering the edge case.

Also pass job_definition=None explicitly on the FAILED path (it was always
None there anyway).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: lino <68745352+LinoGiger@users.noreply.github.com>
@RapidPoseidon
RapidPoseidon merged commit 5b788fa into main Jul 21, 2026
1 check passed
@RapidPoseidon
RapidPoseidon deleted the feat(job)/atomic-job-definition-creation branch July 21, 2026 09:40
RapidPoseidon added a commit that referenced this pull request Jul 21, 2026
)

Concurrent local-file uploads could crash with `OSError: [Errno 24] Too
many open files` on a common `ulimit -n 1024`. The dominant descriptor
consumer is the on-disk upload cache: `FanoutCache(shards=128)` opens
~384 file descriptors at construction and grows to ~640 under the default
25-worker pool (measured), so a single default-config process already sits
close to a 1024 limit before open files and HTTP connections are counted.

Lower the default `cacheShards` 128 -> 32. Cache shards only reduce
SQLite write-lock contention, and cache writes are tiny key->filename
strings written after the network upload — so at the default 25 workers,
32 shards (>= worker count) costs nothing measurable while cutting cache
descriptors from ~640 to ~317, keeping even several concurrent processes
under 1024. `cacheShards` stays decoupled from `maxWorkers`: deriving it
from the worker count would reshuffle the key->shard mapping and silently
invalidate the persistent cross-run upload cache whenever workers changed.

Also make the failure discoverable: `FailedUploadException` now detects
descriptor-exhaustion failures (EMFILE, or the message text as a fallback)
and appends a hint pointing at `RAPIDATA_cacheShards` / `RAPIDATA_maxWorkers`
/ `ulimit -n` and the config docs, and both are documented under a new
"Too many open files" section.

The bounded worker pool already respected the post-#687 atomic
upload-then-persist ordering (it lives entirely within the state machine's
UPLOAD_DATAPOINTS step); this change tunes the cache below it and does not
touch that control flow.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: lino <68745352+LinoGiger@users.noreply.github.com>
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