diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 1b350e139..e4cce1a90 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -253,23 +253,35 @@ other instances may lag behind, or run forks of, dandi-archive. **Support for a new dandi-archive feature must be backward compatible** with prior versions of dandi-archive, in particular those currently deployed by the -DANDI and EMBER instances. A new dandi-cli release must keep working against -all of them, and use the new feature only where the server provides it. This -way dandi-cli can be released before the feature is released or deployed by -dandi-archive, and tested against sandbox instances that already have it. We -do not need synchronized releases of dandi-cli and dandi-archive, or -synchronized deployments across all instances: those are fragile and need +DANDI, EMBER and LINC instances. A new dandi-cli release must keep working +against all of them, and use the new feature only where the server provides +it. This way dandi-cli can be released before the feature is released or +deployed by dandi-archive, and tested against sandbox instances that already +have it. We do not need synchronized releases of dandi-cli and dandi-archive, +or synchronized deployments across all instances: those are fragile and need orchestration. Do not ask instances that lack the feature to reject the new client via `cli-minimal-version` or `cli-bad-versions` in `/api/info/`. +LINC runs a fork of dandi-archive 0.3.115 (`lincbrain/linc-archive`) with +schema version 0.6.9, far behind the others, and is still in use. To see +what the instances deploy: + + for h in api.dandiarchive.org api.sandbox.dandiarchive.org \ + api-dandi.emberarchive.org api-dandi.sandbox.emberarchive.org \ + api.lincbrain.org staging-api.lincbrain.org; do + echo "$h: $(curl -s https://$h/api/info/ \ + | jq -c '{version, schema_version, allowed_schema_versions}')" + done + Decide, per server, whether the feature is available, either: 1. **By server version**, if the dandi-archive release that added the feature is known: compare it with `DandiAPIClient.server_info["version"]` (the `version` field of `/api/info/`). Forks may report versions that cannot be - compared with dandi-archive's (e.g. LINC reported `0.0.post6586+g39387b8`), - so a version check works only for instances deploying dandi-archive - releases. + compared with dandi-archive's (e.g. EMBER reports + `0.0.post1.dev7927+g9e4606812` and LINC `0.0.post6586+g39387b8`), so a + version check works only for instances deploying dandi-archive releases, + i.e. only DANDI's. 2. **By sensing** the feature, which is the most likely case while it is available only on a sandbox instance or in a fork. Probe the server once, or detect support from the response to the first use, remember the result @@ -281,16 +293,19 @@ Decide, per server, whether the feature is available, either: Never decide by the instance name or URL. In addition: - In a comment at the conditional, refer to the dandi-archive issue/PR it - accommodates and say when the fallback can be removed (e.g. "once LINC and - EMBER update their API to match dandi-archive >= 0.12.14"), so that it can - be found, reviewed, and removed later. + accommodates and say when the fallback can be removed (e.g. "once LINC signs + the header for embargoed Zarrs, as dandi-archive >= 0.12.14 does"), so that + it can be found, reviewed, and removed later. - Log use of the fallback at DEBUG or INFO level, not as a warning or error; users cannot do anything about it. - Test both code paths. Integration tests run against the latest `dandiarchive/dandiarchive-api` Docker image (dandi-archive `master`), so test the prior behavior with mocked responses (`responses`), and skip integration tests needing the feature if the server does not support it. -- Remove the fallback once all known instances support the feature. +- Remove the fallback once no known instance needs it: all support the + feature, or those that do not cannot reach the code (e.g. a server whose + schema version is older than the client's never needs + `allowed_schema_versions`). Examples in the code (links are to the 0.81.0 release): diff --git a/dandi/dandiapi.py b/dandi/dandiapi.py index ccf17239b..d168b26c1 100644 --- a/dandi/dandiapi.py +++ b/dandi/dandiapi.py @@ -701,8 +701,7 @@ def check_schema_version(self, schema_version: str | None = None) -> None: number for 0.x series, and same MAJOR version for/after 1.x series; - the same; - higher than the client has, but only if the client's schema version is listed - among the server's `allowed_schema_versions` (as returned by the `/info` API endpoint), - or if not there -- `dandischema.consts.ALLOWED_INPUT_SCHEMAS` is consulted. + among the server's `allowed_schema_versions` (as returned by the `/info` API endpoint). If neither of above, a `SchemaVersionError` is raised. @@ -717,11 +716,10 @@ def check_schema_version(self, schema_version: str | None = None) -> None: schema_version ) if server_ver > our_ver: - # TODO: potentially adjust here if name would be different: see - # https://github.com/dandi/dandi-archive/issues/2624 - allowed_schema_versions = server_info.get( - "allowed_schema_versions", dandischema.consts.ALLOWED_INPUT_SCHEMAS - ) + # A server without the field (dandi-archive < 0.19.0, + # https://github.com/dandi/dandi-archive/pull/2625) accepts no older + # schema. LINC is one, but its schema is older than any client's. + allowed_schema_versions = server_info.get("allowed_schema_versions") or [] if schema_version not in allowed_schema_versions: raise SchemaVersionError( f"Server uses schema version {server_schema_version};" diff --git a/dandi/download.py b/dandi/download.py index a87d5380e..2b8faf748 100644 --- a/dandi/download.py +++ b/dandi/download.py @@ -1213,11 +1213,7 @@ def _check_attempts_and_sleep( lgr.debug("%s - download failed after %d attempts: %s", path, attempt, exc) return 0 if exc.response is not None: - if exc.response.status_code not in ( - 400, # Bad Request, but happened with girder: - # https://github.com/dandi/dandi-cli/issues/87 - *RETRY_STATUSES, - ): + if exc.response.status_code not in RETRY_STATUSES: lgr.debug( "%s - download failed due to response %d: %s", path, diff --git a/dandi/files/zarr.py b/dandi/files/zarr.py index 25394769e..9db1caef4 100644 --- a/dandi/files/zarr.py +++ b/dandi/files/zarr.py @@ -601,6 +601,12 @@ def mkzarr() -> str: json={"name": asset_path, "dandiset": dandiset.identifier}, ) except requests.HTTPError as e: + # dandi-archive >= 1.0.0 rejects a duplicate with the DRF + # "unique set" error + # (https://github.com/dandi/dandi-archive/pull/2589), but still + # answers "Zarr already exists" if it loses a race to a + # concurrent creation, as older versions always do; both must + # stay. if e.response is not None and ( "Zarr already exists" in e.response.text or "dandiset, name must make a unique set" in e.response.text @@ -1008,8 +1014,11 @@ def _upload_zarr_file( if item.content_type is not None: headers["Content-Type"] = item.content_type - # TODO: Replace below implementation with this once LINC and EMBER - # update their API to match dandi-archive >= 0.12.14 + # TODO: Replace below implementation with this once LINC signs the + # header for embargoed Zarrs, as dandi-archive >= 0.12.14 does + # (https://github.com/dandi/dandi-archive/pull/2503). DANDI and EMBER + # do; LINC (a fork of dandi-archive 0.3.115) does not, and accepts + # embargoed Zarrs without it. # if dandiset.embargo_status == EmbargoStatus.EMBARGOED: # # Technically, a header can be specified more than once. # # Join all together just in case. diff --git a/dandi/tests/test_dandiapi.py b/dandi/tests/test_dandiapi.py index 8da28852e..af56413c9 100644 --- a/dandi/tests/test_dandiapi.py +++ b/dandi/tests/test_dandiapi.py @@ -13,6 +13,7 @@ import anys import click import dandischema +from dandischema.consts import ALLOWED_INPUT_SCHEMAS from dandischema.models import UUID_PATTERN, DigestType, get_schema_version from packaging.version import Version as _PackagingVersion import pytest @@ -348,6 +349,8 @@ def test_check_schema_version( ) -> None: server_info = { "schema_version": server_schema_version, + # as dandi-archive reports it + "allowed_schema_versions": ALLOWED_INPUT_SCHEMAS, "version": "0.0.0", "services": { "api": {"url": "https://test.nil/api"}, @@ -391,6 +394,38 @@ def _mock_server_info(schema_version: str) -> None: responses.add(responses.GET, "https://test.nil/api/info/", json=info) +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "allowed_schema_versions,local_schema_version,should_raise", + [ + (["9.9.8", "9.9.9"], "9.9.8", False), + # In the client's own ALLOWED_INPUT_SCHEMAS, but not allowed by the server + (["9.9.8", "9.9.9"], "0.6.9", True), + # A server not reporting the field (dandi-archive < 0.19.0) allows none + (None, "9.9.8", True), + ], +) +@responses.activate +def test_check_schema_version_allowed_by_server( + allowed_schema_versions: list[str] | None, + local_schema_version: str, + should_raise: bool, +) -> None: + info = _server_info("9.9.9") + if allowed_schema_versions is not None: + info["allowed_schema_versions"] = allowed_schema_versions + responses.add(responses.GET, "https://test.nil/server-info", json=info) + responses.add(responses.GET, "https://test.nil/api/info/", json=info) + client = DandiAPIClient("https://test.nil/api") + if should_raise: + with pytest.raises( + SchemaVersionError, match="not among any of the allowed upgradable" + ): + client.check_schema_version(local_schema_version) + else: + client.check_schema_version(local_schema_version) + + @pytest.mark.ai_generated @pytest.mark.parametrize( "server_schema,obj_schema,expected_schema,dropped,kept", diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index 43bb9579f..1494b4428 100644 --- a/dandi/tests/test_download.py +++ b/dandi/tests/test_download.py @@ -1462,6 +1462,24 @@ def test__check_attempts_and_sleep() -> None: assert mock_sleep.call_args.args[0] >= 0 +@pytest.mark.ai_generated +@pytest.mark.parametrize("status_code", [400, 403, 404]) +def test__check_attempts_and_sleep_no_retry(status_code: int) -> None: + response = requests.Response() + response.status_code = status_code + with mock.patch("time.sleep") as mock_sleep: + assert ( + _check_attempts_and_sleep( + Path("some/path"), + HTTPError(response=response), + attempt=1, + attempts_allowed=2, + ) + == 0 + ) + mock_sleep.assert_not_called() + + @pytest.mark.parametrize("status_code", [429, 503]) def test__check_attempts_and_sleep_retries(status_code: int) -> None: f = partial(_check_attempts_and_sleep, Path("some/path")) diff --git a/dandi/utils.py b/dandi/utils.py index cd6ea7afd..eea572911 100644 --- a/dandi/utils.py +++ b/dandi/utils.py @@ -576,11 +576,6 @@ def get_instance(dandi_instance_id: str | DandiInstance) -> DandiInstance: else: dandi_id = dandi_instance_id instance = known_instances[dandi_id] - if dandi_id == "dandi-staging": - raise ValueError( - "'dandi-staging' DANDI instance identifier was removed. " - "The instance was renamed into 'dandi-sandbox', please use that identifier instead." - ) if redirector_url is None: assert instance is not None return _get_instance(instance.api.rstrip("/"), True, instance, dandi_id)