Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1960 +/- ##
==========================================
+ Coverage 91.18% 91.44% +0.26%
==========================================
Files 95 95
Lines 14740 15209 +469
==========================================
+ Hits 13440 13908 +468
- Misses 1300 1301 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
yarikoptic
left a comment
There was a problem hiding this comment.
quick view
- provide compatibility with older dandi-archive's (until EMBER updates; if it already did -- let me know...)
- keep in mind, as per our recent conversation, this is just one of the end-points which causes server-side slow down due to absent "debouncing", but there are others too! (I forgot already what it was but IIRC triggring manifests updates upon addition of assets etc). So I rather see addition of this particular API interface as "band-aid": ideally there should be review of server tasks allocations dependent on adjustment of the state of assets in the dandiset to address the core issue! Implementation of "bulk" operations would still be welcome but would not address it as users could still "delete in bulks of 1"!
Points the dandi-cli "master" matrix job at naglepuff/dandi-cli@mln/use-bulk-delete-endpoint (dandi/dandi-cli#1960). Revert before merging. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
We would work directly with @NEStock to make sure EMBER receives this same update. Since it's adding a new endpoint and major data migrations, etc., won't be required, we anticipate this should be relatively straightforward. That makes the problem of compatibility much easier.
Rather than "debouncing" what we want here is "throttling", which we implement in a web service via rate limiting. Since we're retaining the single-asset delete endpoint, a rate limit on that endpoint prevents a recurrence of the original issue. And putting a rate limit on the bulk deletion endpoint prevents problems of "deleting in bulks of 1". This problem arose due to the scale of a specific dandiset, and the proper fix for that problem really is bulk-delete It is not just a band-aid: deletion of many assets due to a change in the local state of a dandiset can be orders of magnitude faster with bulk-delete, and in that sense this is indeed a change in "server task allocations". |
Points the dandi-cli "master" matrix job at naglepuff/dandi-cli@mln/use-bulk-delete-endpoint (dandi/dandi-cli#1960). Revert before merging. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Points the dandi-cli "master" matrix job at naglepuff/dandi-cli@mln/use-bulk-delete-endpoint (dandi/dandi-cli#1960). Revert before merging. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Points the dandi-cli "master" matrix job at naglepuff/dandi-cli@mln/use-bulk-delete-endpoint (dandi/dandi-cli#1960). Revert before merging. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Points the dandi-cli "master" matrix job at naglepuff/dandi-cli@mln/use-bulk-delete-endpoint (dandi/dandi-cli#1960). Revert before merging. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add `RemoteDandiset.delete_assets()` and `iter_delete_assets()`, which delete assets from a draft version via the archive's bulk deletion endpoint (`POST .../assets/bulk-delete/`), sending batches of at most `BULK_DELETE_BATCH_SIZE` assets one request at a time. The endpoint is all-or-nothing and returns a 404 naming any requested assets that do not belong to the version. By default such assets are dropped from the batch, the rest is resent, and the skipped assets are reported in `BulkDeleteResult.absent`; with `skip_missing=False` an `AssetsNotFoundError` is raised instead. A 404 that did not come from the endpoint (e.g., from a server that predates it) raises `BulkDeleteUnsupportedError`. Both the current message-only 404 body and a structured body with `missing_asset_ids` are supported. Nothing uses the new methods yet. The integration tests require an archive with the bulk deletion endpoint. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`RemoteAsset.delete()` now deletes the asset with a one-asset request to the archive's bulk deletion endpoint instead of the per-asset DELETE endpoint, so the CLI no longer uses the latter. Every caller of `RemoteAsset.delete()` (including `dandi delete`, `upload --sync`, and `move --existing overwrite`) is switched over by this change. Deleting an asset that no longer belongs to the version still raises a 404 error (now `AssetsNotFoundError`). Deleting from a non-draft version now raises `ValueError` before any request is sent. The asset deletion tests in `test_delete.py` now spy on POST requests instead of DELETE requests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
632a83b to
34df1b9
Compare
`dandi delete` now deletes assets with the bulk deletion endpoint in
batches of at most `BULK_DELETE_BATCH_SIZE` assets, sending one request
at a time, instead of one concurrent request per asset. The status of
each asset is reported once its batch has been processed:
- assets that no longer exist on the server by the time they are
deleted (e.g., deleted concurrently by someone else) are reported as
skipped ("already absent") rather than as errors;
- if a batch fails, all of its assets are reported as errors, and
processing continues with the next batch.
As requests are no longer parallelized, the `jobs` argument of
`delete()` is now ignored.
Add a `quiet_statuses` parameter to `RESTFullAPIClient.request()` for
HTTP error statuses that the caller expects and handles, which are then
logged at DEBUG instead of ERROR level. Bulk deletion requests use it
for 404s, so that skipped assets do not produce error log messages.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`upload --sync` now deletes the remote assets that no longer exist locally with `RemoteDandiset.delete_assets()`, which uses the bulk deletion endpoint in batches of at most `BULK_DELETE_BATCH_SIZE` assets, instead of with one request per asset. Assets that no longer exist on the server by the time they are deleted are skipped. The number of deleted (and skipped) assets is now logged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
If a bulk asset deletion request returns a 404 that did not come from the bulk deletion endpoint, delete the assets one at a time using the single-asset deletion endpoint instead. The client remembers this so that further deletions skip the bulk deletion request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The archive's bulk deletion endpoint now reports assets that do not belong to the version with a 409 response whose body lists them in `missing_asset_ids`, instead of a 404 with the IDs embedded in a message string. A 404 from the endpoint therefore no longer needs to be inspected to tell missing assets apart from a server without the endpoint, so any 404 now falls back to single-asset deletion. Drop parsing of the old string message, which was never released. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A 404 from the bulk deletion endpoint means either that the server does not support bulk deletion or that the Dandiset version does not exist. Previously, both were treated as the former, so a missing version led to deleting assets one at a time, with every asset reported as already absent. Now, fetch the version to tell the two cases apart. If the version does not exist, raise `NotFoundError` without falling back or remembering the endpoint as unsupported. The extra request is only made on a 404, and, as the fallback is remembered, at most once per client when the version exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sets The archive's bulk deletion endpoint is no longer all-or-nothing: it deletes those of the requested assets that belong to the version, ignores the rest, and responds with a 200 whose body lists the IDs of the assets it deleted. It no longer responds with a 409 for missing assets. Send a single request per batch and report the requested assets left out of the response as absent, dropping the 409 handling and the retry loop. Raise a `RuntimeError` if the response body is not a list of asset IDs. With `skip_missing=False`, `AssetsNotFoundError` is now raised after the rest of the batch has been deleted. Record the IDs deleted in the batch before the error in its new `deleted_asset_ids` attribute, for both the bulk endpoint and the single-asset fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1aa10bc to
24853e6
Compare
…k deletion When the server does not support bulk deletion, `delete_assets()` falls back to deleting a batch's assets one at a time, but `dandi delete` only reported their statuses once the whole batch was done, and a failure partway through marked every asset in the batch as an error, including those already deleted and those never attempted. Let `_delete_asset_batch()` raise `BulkDeleteUnsupportedError` instead of falling back when given `fallback=False`, and use that in `Deleter.process_assets()` to delete the assets of each batch one at a time itself on such servers, reporting each one's status as soon as it is processed, with a failure only affecting that asset, as before bulk deletion was used. Requests are still sent one at a time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dandi-cli's CI runs the integration tests against the published archive Docker image, which does not have the bulk deletion endpoint yet, so the CLI falls back to single-asset deletion there. Four tests assumed the bulk endpoint: - `test_delete_multiple_batches` and `test_upload_sync_multiple_batches` expected one bulk deletion request per batch. Probe the server with a bulk deletion request for no assets (a 400 with the endpoint, a 404 without it) using a new `bulk_delete_supported()` test helper, and expect either one request per batch or a single one followed by single-asset deletions. - `test_remote_asset_delete` expected a bulk deletion request even when the session-scoped client had already found the endpoint missing. - `test_delete_assets_already_deleted_integration` expected the asset after a missing one to be deleted, which the single-asset fallback does not do; put the missing asset last instead. Also add `test_delete_bulk_reports_per_asset`, which emulates the bulk deletion endpoint so that `dandi delete`'s reporting of successful bulk deletion requests is exercised against any server. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@yarikoptic this is in a state where you are free to unleash the robot reviewers. |
Switches all asset deletion in the CLI to the bulk asset deletion endpoint added in dandi/dandi-archive#2920 (
POST /dandisets/{id}/versions/{version}/assets/bulk-delete/). Issuing many per-assetDELETErequests in quick succession can overload the server.Draft: this depends on dandi/dandi-archive#2920. On servers without the endpoint, the CLI falls back to deleting assets one at a time with the single-asset
DELETE, so it keeps working against older archives. CI here runs against the publisheddandiarchive/dandiarchive-apiDocker image, which doesn't have the endpoint yet, so the integration tests exercise the fallback. They're written to pass against servers with or without the endpoint.Changes
RemoteDandiset.delete_assets()/iter_delete_assets()(new):BULK_DELETE_BATCH_SIZE(100), one request at a time. Input is deduplicated.BulkDeleteResultwithdeletedandabsentlists.absent. This covers concurrent deletion and retries of a request that already succeeded. A response body that isn't a list of IDs raisesRuntimeError.skip_missing=False, it raisesAssetsNotFoundError(withmissing_asset_ids) once the batch is processed. The rest of the batch has already been deleted at that point, and the error lists those assets indeleted_asset_ids.NotFoundError. Otherwise the server predates the endpoint, and the batch is deleted one asset at a time with the single-assetDELETE, with the sameskip_missinghandling (except that withskip_missing=Falseit stops at the first missing asset). The client remembers this, so later batches and calls skip the bulk request.RemoteAsset.delete()now goes through the bulk endpoint with a single ID andskip_missing=False. This also coversmoveand other callers.dandi deletesends assets in batches instead of one request per asset through pyout's worker pool:Skipped.Errorand continues. That's accurate because the server rolls back the whole request.Error, as onmaster, but without the concurrency.Deleter.process_assets_pyout()/process_assets_debug()are replaced by a singleprocess_assets(). The--devel-debugoutput now has one dict per asset with its final status (noDeletinglines).jobsargument ofdelete()is now ignored (thedandi deletecommand doesn't expose it).upload --syncdeletes remote assets that no longer exist locally in batches, and logs separate counts for deleted and already-absent assets. On servers without the endpoint it behaves as onmaster(sequential single-asset deletes, stopping at the first failure), except that assets that are already gone are skipped instead of causing an error.request()gained aquiet_statusesparameter, so the expected 404s from the bulk endpoint, the version check and the fallback are logged at DEBUG instead of ERROR.Not included
dandi movestill renames and overwrites one asset at a time, so an overwrite is still one delete request per asset. It's now through the bulk endpoint, though. Batching those deletes up front would risk local and remote diverging after a partial failure, and the renames cost more than the deletes anyway.Testing
responses) unit tests cover:skip_missingdelete,upload --syncandRemoteAsset.delete(), including multiple batches and assets deleted concurrently. They pass against servers with or without the endpoint:bulk_delete_supported()test helper probes the server with a bulk deletion request for no assets. A server with the endpoint rejects it with a 400; one without it responds with a 404.dandi deletetest emulates a server without the endpoint and checks that a failure only affects that asset. Another emulates a server with it, so that reporting successful bulk requests is covered on either server.bulk-delete-asset-endpoint-syncat08ed76ef):test_dandiapi.py,test_delete.pyandtest_upload.py: 191 passed, 1 xfailed, 0 failed, before the last commit (per-asset reporting in the fallback).test_delete.pyand the deletion tests intest_dandiapi.py: 63 passed, 0 failed.63f2947e), the delete- and sync-related tests in those three files: 76 passed, 0 failed. The server-without-endpoint path of these tests is exercised by this PR's CI.cli-integrationworkflow passed against #2920's final design with this branch ata7148196: https://github.com/dandi/dandi-archive/actions/runs/41507477207 (attempt 3; 318 passed, 2 xfailed).🤖 Generated with Claude Code