Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New AssetPaths lead to dandi-cli integration testing failing #1336

Closed
yarikoptic opened this issue Oct 19, 2022 · 7 comments · Fixed by #1338
Closed

New AssetPaths lead to dandi-cli integration testing failing #1336

yarikoptic opened this issue Oct 19, 2022 · 7 comments · Fixed by #1338
Labels
released This issue/pull request has been released.

Comments

@yarikoptic
Copy link
Member

Merging #1312 seems has lead to failing testing against dandi-cli in master

  • Given that it was green within PR, please check if that "integration" testing is actually using the code from the PR
  • Tests trigger 500 from the API (this is just a sample, didn't check other, total it was 10 of them)
requests.exceptions.HTTPError: 500 Server Error: Internal Server Error for url: http://localhost:8000/api/dandisets/000224/versions/draft/assets/05ccca9a-7381-4666-b043-c4d2408eb334/
@TheChymera
Copy link

I seem to get this error on dandi/dandi-cli#1104 (PR against dandi-cli master, as described here). There are indeed 10 test failures, but only 9 are requests.exceptions.HTTPError.

Example traceback:

Traceback (most recent call last):
  File "/home/runner/work/dandi-cli/dandi-cli/dandi/dandiapi.py", line 222, in request
    reraise=True,
  File "/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/tenacity/__init__.py", line 384, in __iter__
    do = self.iter(retry_state=retry_state)
  File "/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/tenacity/__init__.py", line 362, in iter
    raise retry_exc.reraise()
  File "/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/tenacity/__init__.py", line 195, in reraise
    raise self.last_attempt.result()
  File "/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/concurrent/futures/_base.py", line 428, in result
    return self.__get_result()
  File "/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/concurrent/futures/_base.py", line 384, in __get_result
    raise self._exception
  File "/home/runner/work/dandi-cli/dandi-cli/dandi/dandiapi.py", line 241, in request
    result.raise_for_status()
  File "/opt/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/site-packages/requests/models.py", line 1021, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 500 Server Error: Internal Server Error for url: http://localhost:8000/api/dandisets/000224/versions/draft/assets/7e8d0efd-86d9-4a05-b2de-7ee95a349c5a/

The different error is an AssertionError at the test level, i.e. the process succeeds but not with the expected output:

____________________________ test_delete_zarr_path _____________________________
/home/runner/work/dandi-cli/dandi-cli/dandi/tests/test_delete.py:438: in test_delete_zarr_path
    assert list_paths(tmp_path) == [
  AssertionError: assert [PosixPath('/...arr/arr_1/0')] == [PosixPath('/...ndiset.yaml')]
    Left contains 2 more items, first extra item: PosixPath('/tmp/pytest-of-runner/pytest-0/test_delete_zarr_path0/000066/sample.zarr/arr_0/0')
    Full diff:
      [
       PosixPath('/tmp/pytest-of-runner/pytest-0/test_delete_zarr_path0/000066/dandiset.yaml'),
    +  PosixPath('/tmp/pytest-of-runner/pytest-0/test_delete_zarr_path0/000066/sample.zarr/arr_0/0'),
    +  PosixPath('/tmp/pytest-of-runner/pytest-0/test_delete_zarr_path0/000066/sample.zarr/arr_1/0'),
      ]

@jwodder
Copy link
Member

jwodder commented Oct 20, 2022

I can confirm that the tests are failing when run locally. Here is a gist of the logs from the Django container during a complete test run; grep it for errors.

All the failures seem to boil down to one of two API calls: deleting a Zarr and modifying the asset metadata for a Zarr.

MVCEs for Failures

Deleting a Zarr

(Note: This script originally tried to clean up afterwards by deleting the Dandiset, but that failed with 500 as well.)

import logging
import os
from pathlib import Path
import tempfile
from dandi.dandiapi import DandiAPIClient
from dandi.files import dandi_file
import numpy as np
import zarr

logging.basicConfig(
    format="%(asctime)s [%(levelname)-8s] %(name)s: %(message)s",
    datefmt="%H:%M:%S",
    level=logging.DEBUG,
)

with DandiAPIClient.for_dandi_instance(
    "dandi-staging", token=os.environ["DANDI_API_KEY"]
) as client:
    d = client.create_dandiset(
        "Dandiset with an empty Zarr",
        {
            "schemaKey": "Dandiset",
            "name": "Test Dandiset",
            "description": "A test Dandiset",
            "contributor": [
                {
                    "schemaKey": "Person",
                    "name": "Wodder, John",
                    "roleName": ["dcite:Author", "dcite:ContactPerson"],
                }
            ],
            "license": ["spdx:CC0-1.0"],
        },
    )

    dandiset_id = d.identifier
    print("DANDISET ID:", dandiset_id)

    with tempfile.TemporaryDirectory() as tmpdir:
        filepath = Path(tmpdir, "example.zarr")
        zarr.save(filepath, np.arange(1000), np.arange(1000, 0, -1))
        zf = dandi_file(filepath)
        asset = zf.upload(d, {"description": "A test Zarr"})

    asset.delete()

Modifying a Zarr's metadata

import logging
import os
from pathlib import Path
import tempfile
from dandi.dandiapi import DandiAPIClient
from dandi.files import dandi_file
import numpy as np
import zarr

logging.basicConfig(
    format="%(asctime)s [%(levelname)-8s] %(name)s: %(message)s",
    datefmt="%H:%M:%S",
    level=logging.DEBUG,
)

log = logging.getLogger()

with DandiAPIClient.for_dandi_instance(
    "dandi-staging", token=os.environ["DANDI_API_KEY"]
) as client:
    d = client.create_dandiset(
        "Dandiset with an empty Zarr",
        {
            "schemaKey": "Dandiset",
            "name": "Test Dandiset",
            "description": "A test Dandiset",
            "contributor": [
                {
                    "schemaKey": "Person",
                    "name": "Wodder, John",
                    "roleName": ["dcite:Author", "dcite:ContactPerson"],
                }
            ],
            "license": ["spdx:CC0-1.0"],
        },
    )

    dandiset_id = d.identifier
    log.info("*** Dandiset ID: %s", dandiset_id)

    with tempfile.TemporaryDirectory() as tmpdir:
        filepath = Path(tmpdir, "example.zarr")
        zarr.save(filepath, np.arange(1000), np.arange(1000, 0, -1))
        zf = dandi_file(filepath)
        asset = zf.upload(d, {"description": "A test Zarr"})

    md = asset.get_raw_metadata()
    md["description"] = "modified"
    log.info("*** Modifying asset metadata ...")
    asset.set_raw_metadata(md)

@jjnesbitt
Copy link
Member

Thanks for all of the info, I've determined the issue. I'll be making a pull request to fix it soon, detailing the cause there.

@yarikoptic
Copy link
Member Author

@jwodder did you figure out why the integration testing did not fail in that PR and started to fail only after being merged?

@jwodder
Copy link
Member

jwodder commented Oct 21, 2022

@yarikoptic I think it's the fault of dandi/dandi-cli#1121; because the dandi-cli tests now pull the latest image from Docker Hub, the integration tests don't use the image built from the PR code.

@yarikoptic
Copy link
Member Author

Nice digging, sounds like the likely cause. I guess let's do dandi/dandi-cli#1140 and then use it in the workflow here to disable that pulling.

@dandibot
Copy link
Member

dandibot commented Nov 7, 2022

🚀 Issue was released in v0.3.0 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants