Skip to content

Commit

Permalink
Remove bespoke error-handling code in RemoteButler
Browse files Browse the repository at this point in the history
Remove one-off code handling FileNotFoundError and 404s in RemoteButler, replacing it with the new error propagation framework.
  • Loading branch information
dhirving committed Mar 6, 2024
1 parent 3c4c839 commit 78d6fbc
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 91 deletions.
1 change: 1 addition & 0 deletions python/lsst/daf/butler/_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

"""Specialized Butler exceptions."""
__all__ = (
"ButlerLookupError",
"ButlerUserError",
"DatasetTypeNotSupportedError",
"EmptyQueryResultError",
Expand Down
4 changes: 2 additions & 2 deletions python/lsst/daf/butler/direct_butler.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
from ._dataset_ref import DatasetRef
from ._dataset_type import DatasetType
from ._deferredDatasetHandle import DeferredDatasetHandle
from ._exceptions import ValidationError
from ._exceptions import ButlerLookupError, ValidationError
from ._limited_butler import LimitedButler
from ._registry_shim import RegistryShim
from ._storage_class import StorageClass, StorageClassFactory
Expand Down Expand Up @@ -881,7 +881,7 @@ def _findDatasetRef(
else:
if collections is None:
collections = self._registry.defaults.collections
raise LookupError(
raise ButlerLookupError(
f"Dataset {datasetType.name} with data ID {dataId} "
f"could not be found in collections {collections}."
)
Expand Down
55 changes: 19 additions & 36 deletions python/lsst/daf/butler/remote_butler/_remote_butler.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
from .._dataset_ref import DatasetId, DatasetRef, SerializedDatasetRef
from .._dataset_type import DatasetType, SerializedDatasetType
from .._deferredDatasetHandle import DeferredDatasetHandle
from .._exceptions import create_butler_user_error
from .._exceptions import ButlerLookupError, create_butler_user_error
from .._storage_class import StorageClass, StorageClassFactory
from .._utilities.locked_object import LockedObject
from ..datastore import DatasetRefURIs
Expand Down Expand Up @@ -246,14 +246,9 @@ def getDeferred(
storageClass: str | StorageClass | None = None,
**kwargs: Any,
) -> DeferredDatasetHandle:
try:
response = self._get_file_info(datasetRefOrType, dataId, collections, kwargs)
# Check that artifact information is available.
_to_file_payload(response)
except FileNotFoundError as e:
# Inconsistent with the behavior of get(), DirectButler returns
# LookupError if a dataset cannot be found here.
raise LookupError(str(e)) from e
response = self._get_file_info(datasetRefOrType, dataId, collections, kwargs)
# Check that artifact information is available.
_to_file_payload(response)
ref = DatasetRef.from_simple(response.dataset_ref, universe=self.dimensions)
return DeferredDatasetHandle(butler=self, ref=ref, parameters=parameters, storageClass=storageClass)

Expand All @@ -269,7 +264,10 @@ def get(
**kwargs: Any,
) -> Any:
# Docstring inherited.
model = self._get_file_info(datasetRefOrType, dataId, collections, kwargs)
try:
model = self._get_file_info(datasetRefOrType, dataId, collections, kwargs)
except ButlerLookupError as e:
raise FileNotFoundError(str(e)) from e

ref = DatasetRef.from_simple(model.dataset_ref, universe=self.dimensions)
# If the caller provided a DatasetRef, they may have overridden the
Expand Down Expand Up @@ -307,18 +305,11 @@ def _get_file_info(
collections=self._normalize_collections(collections),
data_id=self._simplify_dataId(dataId, kwargs),
)
response = self._post("get_file_by_data_id", request, expected_errors=(404,))
if response.status_code == 404:
raise FileNotFoundError(
f"Dataset not found with DataId: {dataId} DatasetType: {datasetRefOrType}"
f" collections: {collections}"
)
response = self._post("get_file_by_data_id", request)
return self._parse_model(response, GetFileResponseModel)

def _get_file_info_for_ref(self, ref: DatasetRef) -> GetFileResponseModel:
response = self._get(f"get_file/{ref.id}", expected_errors=(404,))
if response.status_code == 404:
raise FileNotFoundError(f"Dataset not found: {ref.id}")
response = self._get(f"get_file/{ref.id}")
return self._parse_model(response, GetFileResponseModel)

def getURIs(
Expand Down Expand Up @@ -463,7 +454,7 @@ def exists(
response = self._get_file_info(
dataset_ref_or_type, dataId=data_id, collections=collections, kwargs=kwargs
)
except FileNotFoundError:
except ButlerLookupError:
return DatasetExistence.UNRECOGNIZED

if response.artifact is None:
Expand Down Expand Up @@ -601,22 +592,19 @@ def _get_url(self, path: str, version: str = "v1") -> str:
slash = "" if self._server_url.endswith("/") else "/"
return f"{self._server_url}{slash}{version}/{path}"

def _post(self, path: str, model: BaseModel, expected_errors: Iterable[int] = ()) -> httpx.Response:
def _post(self, path: str, model: BaseModel) -> httpx.Response:
"""Send a POST request to the Butler server."""
json = model.model_dump_json(exclude_unset=True).encode("utf-8")
return self._send_request(
"POST",
path,
content=json,
headers={"content-type": "application/json"},
expected_errors=expected_errors,
)

def _get(
self, path: str, params: Mapping[str, str | bool] | None = None, expected_errors: Iterable[int] = ()
) -> httpx.Response:
def _get(self, path: str, params: Mapping[str, str | bool] | None = None) -> httpx.Response:
"""Send a GET request to the Butler server."""
return self._send_request("GET", path, params=params, expected_errors=expected_errors)
return self._send_request("GET", path, params=params)

def _send_request(
self,
Expand All @@ -626,7 +614,6 @@ def _send_request(
content: bytes | None = None,
params: Mapping[str, str | bool] | None = None,
headers: Mapping[str, str] | None = None,
expected_errors: Iterable[int],
) -> httpx.Response:
url = self._get_url(path)

Expand All @@ -649,15 +636,11 @@ def _send_request(
exc = create_butler_user_error(model.error_type, model.detail)
exc.add_note(f"Client request ID: {request_id}")
raise exc
else:
# Server sent an expected error code, but the body wasn't
# in the expected JSON format. This likely means some HTTP
# thing between us and the server is misbehaving.
response.raise_for_status()

if response.status_code not in expected_errors:
response.raise_for_status()
# If model is None, server sent an expected error code, but the
# body wasn't in the expected JSON format. This likely means
# some HTTP thing between us and the server is misbehaving.

response.raise_for_status()
return response
except httpx.HTTPError as e:
raise ButlerServerError(request_id) from e
Expand Down Expand Up @@ -771,7 +754,7 @@ def _apply_storage_class_override(
def _to_file_payload(get_file_response: GetFileResponseModel) -> FileDatastoreGetPayload:
if get_file_response.artifact is None:
ref = get_file_response.dataset_ref
raise FileNotFoundError(f"Dataset is known, but artifact is not available. (datasetId='{ref.id}')")
raise ButlerLookupError(f"Dataset is known, but artifact is not available. (datasetId='{ref.id}')")

return get_file_response.artifact

Expand Down
41 changes: 0 additions & 41 deletions python/lsst/daf/butler/remote_butler/server/_exceptions.py

This file was deleted.

21 changes: 9 additions & 12 deletions python/lsst/daf/butler/remote_butler/server/handlers/_external.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@
GetFileResponseModel,
)

from ...._exceptions import ButlerLookupError
from .._dependencies import factory_dependency
from .._exceptions import NotFoundException
from .._factory import Factory

external_router = APIRouter()
Expand Down Expand Up @@ -157,7 +157,7 @@ def get_file(
butler = factory.create_butler()
ref = butler.get_dataset(dataset_id, datastore_records=True)
if ref is None:
raise NotFoundException(f"Dataset ID {dataset_id} not found")
raise ButlerLookupError(f"Dataset ID {dataset_id} not found")
return _get_file_by_ref(butler, ref)


Expand All @@ -171,16 +171,13 @@ def get_file_by_data_id(
factory: Factory = Depends(factory_dependency),
) -> GetFileResponseModel:
butler = factory.create_butler()
try:
ref = butler._findDatasetRef(
datasetRefOrType=request.dataset_type_name,
dataId=request.data_id,
collections=request.collections,
datastore_records=True,
)
return _get_file_by_ref(butler, ref)
except LookupError as e:
raise NotFoundException() from e
ref = butler._findDatasetRef(
datasetRefOrType=request.dataset_type_name,
dataId=request.data_id,
collections=request.collections,
datastore_records=True,
)
return _get_file_by_ref(butler, ref)


def _get_file_by_ref(butler: Butler, ref: DatasetRef) -> GetFileResponseModel:
Expand Down

0 comments on commit 78d6fbc

Please sign in to comment.