From fc2035ac6092a30d2fa28b11a80c7395ae5005c2 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Wed, 3 Aug 2022 10:57:17 -0400 Subject: [PATCH 1/3] RF: establish .support.typing for all needed version conditional imports for type checking --- dandi/metadata.py | 6 +----- dandi/support/typing.py | 6 ++++++ dandi/tests/fixtures.py | 10 ++++------ dandi/upload.py | 6 +----- 4 files changed, 12 insertions(+), 16 deletions(-) create mode 100644 dandi/support/typing.py diff --git a/dandi/metadata.py b/dandi/metadata.py index c1a13a60c..b9cbeec21 100644 --- a/dandi/metadata.py +++ b/dandi/metadata.py @@ -7,7 +7,6 @@ import os.path as op from pathlib import Path import re -import sys from typing import ( TYPE_CHECKING, Any, @@ -664,10 +663,7 @@ def extract_field(field: str, metadata: dict) -> Any: if TYPE_CHECKING: - if sys.version_info >= (3, 8): - from typing import TypedDict - else: - from typing_extensions import TypedDict + from .support.typing import TypedDict class Neurodatum(TypedDict): module: str diff --git a/dandi/support/typing.py b/dandi/support/typing.py new file mode 100644 index 000000000..fb138f98a --- /dev/null +++ b/dandi/support/typing.py @@ -0,0 +1,6 @@ +import sys + +if sys.version_info >= (3, 8): + from typing import Literal, TypedDict # noqa: F401 +else: + from typing_extensions import Literal, TypedDict # noqa: F401 diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 095d54943..38b05cb0f 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -8,7 +8,6 @@ import re import shutil from subprocess import DEVNULL, check_output, run -import sys import tempfile from time import sleep from typing import TYPE_CHECKING, Any, Callable, Dict, Iterator, List, Optional, Union @@ -179,10 +178,7 @@ def organized_nwb_dir2( if TYPE_CHECKING: - if sys.version_info >= (3, 8): - from typing import Literal - else: - from typing_extensions import Literal + from ..support.typing import Literal Scope = Union[ Literal["session"], @@ -448,7 +444,9 @@ def bids_dandiset(new_dandiset: SampleDandiset, bids_examples: str) -> SampleDan @pytest.fixture() -def bids_dandiset_invalid(new_dandiset: SampleDandiset, bids_examples: str) -> SampleDandiset: +def bids_dandiset_invalid( + new_dandiset: SampleDandiset, bids_examples: str +) -> SampleDandiset: copytree( os.path.join(bids_examples, "invalid_pet001"), str(new_dandiset.dspath) + "/", diff --git a/dandi/upload.py b/dandi/upload.py index 6737d6d2e..174d1ccf6 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -3,7 +3,6 @@ import os.path from pathlib import Path import re -import sys import time from typing import TYPE_CHECKING, Any, Dict, Iterator, List, Optional, Set, Tuple, Union @@ -26,10 +25,7 @@ from .utils import ensure_datetime, get_instance, pluralize if TYPE_CHECKING: - if sys.version_info >= (3, 8): - from typing import TypedDict - else: - from typing_extensions import TypedDict + from .support.typing import TypedDict class Uploaded(TypedDict): size: int From 4001ad51d9a28a3be8b104c5a4874ca71daaca05 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Wed, 11 May 2022 14:03:39 -0400 Subject: [PATCH 2/3] BF: (optionally) guard download_generator to not propagate errors This way instead of dumping all tracebacks at the end, some times for benign FileExistsError -- they will be displayed neatly in pyout and also output in detail in the log by default in the case of pyout output handling. But in "debug" mode we will not do anything like that. Closes https://github.com/dandi/dandi-cli/issues/1007 --- dandi/download.py | 27 +++++++++++++++++++++++++-- dandi/tests/test_download.py | 13 ++++++++++--- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/dandi/download.py b/dandi/download.py index b4606574a..7e5c7c6bb 100644 --- a/dandi/download.py +++ b/dandi/download.py @@ -53,6 +53,7 @@ from .support.digests import get_digest, get_zarr_checksum from .support.iterators import IteratorWithAggregation from .support.pyout import naturalsize +from .support.typing import Literal from .utils import ( abbrev_prompt, ensure_datetime, @@ -130,6 +131,7 @@ def download( get_metadata=get_metadata, get_assets=get_assets, jobs_per_zarr=jobs_per_zarr, + on_error="yield" if format == "pyout" else "raise", **kw, ) @@ -207,6 +209,7 @@ def download_generator( get_metadata: bool = True, get_assets: bool = True, jobs_per_zarr: Optional[int] = None, + on_error: Literal["raise", "yield"] = "raise", ) -> Iterator[dict]: """A generator for downloads of files, folders, or entire dandiset from DANDI (as identified by URL) @@ -309,13 +312,33 @@ def download_generator( lock=lock, ) + # If exception is raised we might just raise it, or yield + # an error record + gen = { + "raise": _download_generator, + "yield": _download_generator_guard(path, _download_generator), + }[on_error] + if yield_generator_for_fields: - yield {"path": path, yield_generator_for_fields: _download_generator} + yield {"path": path, yield_generator_for_fields: gen} else: - for resp in _download_generator: + for resp in gen: yield dict(resp, path=path) +def _download_generator_guard(path: str, generator: Iterator[dict]) -> Iterator[dict]: + try: + for resp in generator: + yield dict(resp, path=path) + except Exception as exc: + lgr.error("Caught while downloading %s", path, exc_info=exc) + yield { + "path": path, + "status": "error", + "message": str(exc.__class__.__name__), + } + + class ItemsSummary: """A helper "structure" to accumulate information about assets to be downloaded diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index 560d80639..4734bac62 100644 --- a/dandi/tests/test_download.py +++ b/dandi/tests/test_download.py @@ -32,7 +32,9 @@ "https://dandiarchive.org/dandiset/000027/draft", ], ) -def test_download_000027(url: str, tmp_path: Path) -> None: +def test_download_000027( + url: str, tmp_path: Path, capsys: pytest.CaptureFixture +) -> None: ret = download(url, tmp_path) # type: ignore[func-returns-value] assert not ret # we return nothing ATM, might want to "generate" dsdir = tmp_path / "000027" @@ -48,9 +50,14 @@ def test_download_000027(url: str, tmp_path: Path) -> None: Digester(["md5"])(dsdir / "sub-RAT123" / "sub-RAT123.nwb")["md5"] == "33318fd510094e4304868b4a481d4a5a" ) - # redownload - since already exist there should be an exception + # redownload - since already exist there should be an exception if we are + # not using pyout with pytest.raises(FileExistsError): - download(url, tmp_path) + download(url, tmp_path, format="debug") + assert "FileExistsError" not in capsys.readouterr().out + # but no exception is raised, and rather it gets output to pyout otherwise + download(url, tmp_path) + assert "FileExistsError" in capsys.readouterr().out # TODO: somehow get that status report about what was downloaded and what not download(url, tmp_path, existing="skip") # TODO: check that skipped From f72c2a808c41568043a52d389bfde71ef5936b2f Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Wed, 3 Aug 2022 11:37:54 -0400 Subject: [PATCH 3/3] Suggestions from code review by @jwodder Co-authored-by: John T. Wodder II --- dandi/download.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/dandi/download.py b/dandi/download.py index 7e5c7c6bb..dcdebe415 100644 --- a/dandi/download.py +++ b/dandi/download.py @@ -328,12 +328,10 @@ def download_generator( def _download_generator_guard(path: str, generator: Iterator[dict]) -> Iterator[dict]: try: - for resp in generator: - yield dict(resp, path=path) + yield from generator except Exception as exc: - lgr.error("Caught while downloading %s", path, exc_info=exc) + lgr.exception("Caught while downloading %s:", path) yield { - "path": path, "status": "error", "message": str(exc.__class__.__name__), }