feat: expose custom_metadata_checksum and ConcurrencyConflictError for optimistic concurrency control [PYSDK-130]#632
Open
akunft wants to merge 7 commits into
Open
Conversation
akunft
added
skip:test:long_running
There was a problem hiding this comment.
Pull request overview
This PR adds optimistic concurrency control support for run/item custom metadata updates by introducing an optional checksum parameter and surfacing a dedicated conflict error for HTTP 412 responses.
Changes:
- Added keyword-only
custom_metadata_checksumplumbing from CLI → application service → platformRun.update_*_custom_metadata()calls. - Added
ConcurrencyConflictError(ValueError)and mapped HTTP 412 (precondition failed) to this error in the application service layer. - Updated and extended unit tests to validate checksum forwarding and 412→
ConcurrencyConflictErrorbehavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/aignostics/application/service_test.py | Updates assertions for the new checksum argument and adds tests for checksum forwarding + HTTP 412 mapping. |
| src/aignostics/system/_exceptions.py | Introduces ConcurrencyConflictError for optimistic concurrency conflicts. |
| src/aignostics/platform/resources/runs.py | Adds custom_metadata_checksum kw-only parameter and forwards it into the OpenAPI request model. |
| src/aignostics/application/_service.py | Accepts checksum in service APIs and converts HTTP 412 ApiException into ConcurrencyConflictError. |
| src/aignostics/application/_cli.py | Adds --checksum option to metadata update commands and handles ConcurrencyConflictError. |
Codecov Report❌ Patch coverage is
|
akunft
requested review from
a team and
helmut-hoffer-von-ankershoffen
as code owners
PR #632 Code Review CompleteComprehensive review of optimistic concurrency control implementation. Executive SummaryThis PR successfully implements optimistic concurrency control via an optional Overall Assessment: High-quality implementation with one minor fix required Critical Findings (BLOCKING)
|
claude
Bot
added
the
claude:review:failed
|
…pdate_item_custom_metadata Adds optional optimistic concurrency control to run and item custom metadata updates. When `custom_metadata_checksum` is provided the server rejects the write with HTTP 412 if the metadata was modified since the checksum was read; pass `None` (default) to skip the check. Changes: - platform/resources/runs.py: add keyword-only `custom_metadata_checksum` param to `Run.update_custom_metadata` and `Run.update_item_custom_metadata`, forwarded to `CustomMetadataUpdateRequest` - application/_service.py: propagate `custom_metadata_checksum` through `ApplicationService.application_run_update_custom_metadata`, `application_run_update_item_custom_metadata`, and both static wrappers; handle HTTP 412 explicitly as `ValueError` with a clear concurrency-conflict message instead of the generic `RuntimeError` fallback - tests: add unit tests for checksum forwarding (None and non-None) at the `Run` resource layer; add service-layer tests for checksum propagation and 412→ValueError mapping; fix stale assertions to include `custom_metadata_checksum=None`
- Re-export ConcurrencyConflictError from aignostics.system public API - Update imports in _service.py and _cli.py to use public path - Add unit tests for CLI --checksum success path and ConcurrencyConflictError handler on both update-metadata and update-item-metadata commands
Extract repeated string literals into named constants:
- APPLICATION_CLI_SERVICE_PATCH_TARGET for the service patch path (5 uses)
- _TEST_METADATA_JSON for the metadata stub (4 uses)
- cast("dict[str, Any]") → cast(dict[str, Any]) in runs.py (4 uses)
Replace internal module imports with public API paths: - aignostics.platform.resources.runs.ApiException → aignostics.platform - aignostics.system._exceptions.ConcurrencyConflictError → aignostics.system
…exit code 3, GUI handler
- Move ConcurrencyConflictError from system/_exceptions to platform/_exceptions
and re-export from aignostics.platform — eliminates application→system
bidirectional dependency
- Revert cast() string-form regression in runs.py (restore TC006-compliant
cast("dict[str, Any]", ...) in all four call sites)
- CLI conflict handlers now exit 3 (distinct from exit 2 for not-found)
- Add specific ConcurrencyConflictError handler in GUI metadata editor
with "reload and retry" guidance instead of generic error message
- Update all imports and test assertions accordingly
- Fix parquet row-count bug under pyarrow 23+ (Python 3.14): columns=[]
returns 0 rows; switch to columns=["geometry"] which is engine-agnostic
- Fix GUI metadata save never forwarding checksum: add
custom_metadata_checksum=run_data.custom_metadata_checksum to the
nicegui_run.io_bound call so ConcurrencyConflictError is reachable
- Add --show-checksum flag to dump-metadata and dump-item-metadata CLI
commands; default output unchanged (bare dict), flag emits wrapped
{custom_metadata, custom_metadata_checksum} for read→update loop
- Remove dead SPOT_4_* constants (unreferenced former SPOT_1 values)
- Add unit tests: parquet row-count engine-agnostic guard, --show-checksum
default/wrapped output for both dump commands (4 CLI + 1 utils test)
akunft
force-pushed
the
feat/expose-custom-metadata-checksum-param
branch
from
2e8c224 to
5f22b21
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🛡️ Implements PYSDK-130 following CC-SOP-01 Change Control, part of our ISO 13485-certified QMS | Ketryx Project
Summary
custom_metadata_checksumkeyword-only parameter toRun.update_custom_metadata(),Run.update_item_custom_metadata()(platform layer), all four service-layer methods, and both CLI commands (run update-metadata --checksum,run update-item-metadata --checksum). Defaults toNone— fully backward compatible.ConcurrencyConflictError(ValueError)inplatform/_exceptions.py; HTTP 412 from the server now raises this instead of a genericValueError, letting callers distinguish concurrency conflicts from invalid-ID errors. CLI exits with code 3 on conflict.custom_metadata_checksumto the service call, making the existingConcurrencyConflictErrorhandler reachable and preventing silent lost updates.--show-checksumflag torun dump-metadataandrun dump-item-metadataCLI commands. Default output unchanged (barecustom_metadatadict). With the flag, emits{"custom_metadata": {...}, "custom_metadata_checksum": "..."}— enabling a first-class read→update loop without a separate API call.assert_parquet_geojson_parity(columns=[]→columns=["geometry"]) —columns=[]returns 0 rows under pyarrow 23.0.1 (Python 3.14 engine), breaking e2e parity checks.SPOT_4_*test constants (unreferenced).Test plan
make lintpasses (mypy, pyright, ruff)uv run pytest -m unit— 719 passed, 0 failuresuv run aignostics application run dump-metadata --helpshows--show-checksumoptionuv run aignostics application run dump-item-metadata --helpshows--show-checksumoptionuv run aignostics application run update-metadata --helpshows--checksumoptionuv run aignostics application run update-item-metadata --helpshows--checksumoptionrun dump-metadata RUN_ID --show-checksum→ copy checksum →run update-metadata RUN_ID '{...}' --checksum <X>succeeds; repeat with stale checksum → exits 3 with "modified by another process"Posted by Claude claude-sonnet-4-6 via Claude Code, applying skills cc-sop-01 on behalf of Andreas Kunft (andreas@aignostics.com)