Skip to content

Commit

Permalink
Break RegistryDefaults circular dependency
Browse files Browse the repository at this point in the history
The various Butler sub-objects (registry, collections) need to access the registry defaults.  Added a small helper class so that they don't need a reference to RemoteButler or each other in order to fetch the defaults.
  • Loading branch information
dhirving committed Sep 9, 2024
1 parent f6dc1c9 commit f50f5cb
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 35 deletions.
60 changes: 60 additions & 0 deletions python/lsst/daf/butler/remote_butler/_defaults.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# This file is part of daf_butler.
#
# Developed for the LSST Data Management System.
# This product includes software developed by the LSST Project
# (http://www.lsst.org).
# See the COPYRIGHT file at the top-level directory of this distribution
# for details of code ownership.
#
# This software is dual licensed under the GNU General Public License and also
# under a 3-clause BSD license. Recipients may choose which of these licenses
# to use; please see the files gpl-3.0.txt and/or bsd_license.txt,
# respectively. If you choose the GPL option then the following text applies
# (but note that there is still no warranty even if you opt for BSD instead):
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.

from ..registry import RegistryDefaults


class DefaultsHolder:
"""Holds a `RegistryDefaults` object and allows it to be set.
Parameters
----------
defaults : `RegistryDefaults`
Initial value for the defaults object.
Notes
-----
This exists to work around circular dependency issues (RemoteButler,
ButlerCollections, and Registry all need to know/modify the defaults.)
"""

def __init__(self, defaults: RegistryDefaults) -> None:
self._defaults = defaults

def get(self) -> RegistryDefaults:
"""Retrieve the current registry defaults."""
return self._defaults

def set(self, defaults: RegistryDefaults) -> None:
"""Set a new value for the registry defaults.
Parameters
----------
defaults : `RegistryDefaults`
New value for defaults object.
"""
self._defaults = defaults
14 changes: 9 additions & 5 deletions python/lsst/daf/butler/remote_butler/_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
from collections.abc import Iterable, Iterator, Mapping, Sequence
from typing import Any

from lsst.daf.butler import Butler
from lsst.utils.iteration import ensure_iterable

from .._collection_type import CollectionType
Expand Down Expand Up @@ -69,8 +70,8 @@
convert_collection_arg_to_glob_string_list,
convert_dataset_type_arg_to_glob_string_list,
)
from ._defaults import DefaultsHolder
from ._http_connection import RemoteButlerHttpConnection, parse_model
from ._remote_butler import RemoteButler
from .registry._query_common import CommonQueryArguments
from .registry._query_data_coordinates import QueryDriverDataCoordinateQueryResults
from .registry._query_datasets import QueryDriverDatasetRefQueryResults
Expand All @@ -92,15 +93,18 @@ class RemoteButlerRegistry(Registry):
Parameters
----------
butler : `RemoteButler`
butler : `Butler`
Butler instance to which this registry delegates operations.
defaults : `DefaultHolder`
Reference to object containing default collections and data ID.
connection : `RemoteButlerHttpConnection`
HTTP connection to Butler server for looking up data.
"""

def __init__(self, butler: RemoteButler, connection: RemoteButlerHttpConnection):
def __init__(self, butler: Butler, defaults: DefaultsHolder, connection: RemoteButlerHttpConnection):
self._butler = butler
self._connection = connection
self._defaults = defaults

def isWriteable(self) -> bool:
return self._butler.isWriteable()
Expand All @@ -111,12 +115,12 @@ def dimensions(self) -> DimensionUniverse:

@property
def defaults(self) -> RegistryDefaults:
return self._butler._registry_defaults
return self._defaults.get()

@defaults.setter
def defaults(self, value: RegistryDefaults) -> None:
value.finish(self)
self._butler._registry_defaults = value
self._defaults.set(value)

def refresh(self) -> None:
# In theory the server should manage all necessary invalidation of
Expand Down
34 changes: 13 additions & 21 deletions python/lsst/daf/butler/remote_butler/_remote_butler.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,11 @@
from ..queries import Query
from ..registry import CollectionArgType, NoDefaultCollectionError, Registry, RegistryDefaults
from ._collection_args import convert_collection_arg_to_glob_string_list
from ._defaults import DefaultsHolder
from ._http_connection import RemoteButlerHttpConnection, parse_model, quote_path_variable
from ._query_driver import RemoteQueryDriver
from ._ref_utils import apply_storage_class_override, normalize_dataset_type_name, simplify_dataId
from ._registry import RemoteButlerRegistry
from ._remote_butler_collections import RemoteButlerCollections
from .server_models import (
CollectionList,
Expand All @@ -81,8 +84,6 @@
from ..dimensions import DataId
from ..transfers import RepoExportContext

from ._http_connection import RemoteButlerHttpConnection, parse_model, quote_path_variable


class RemoteButler(Butler): # numpydoc ignore=PR02
"""A `Butler` that can be used to connect through a remote server.
Expand All @@ -109,10 +110,10 @@ class RemoteButler(Butler): # numpydoc ignore=PR02
`Butler.from_config` or `RemoteButlerFactory`.
"""

_registry_defaults: RegistryDefaults
_registry_defaults: DefaultsHolder
_connection: RemoteButlerHttpConnection
_cache: RemoteButlerCache
_registry: Registry
_registry: RemoteButlerRegistry
_datastore_cache_manager: AbstractDatastoreCacheManager | None
_use_disabled_datastore_cache: bool

Expand Down Expand Up @@ -140,15 +141,10 @@ def __new__(
self._datastore_cache_manager = None
self._use_disabled_datastore_cache = use_disabled_datastore_cache

# Avoid a circular import by deferring this import.
from ._registry import RemoteButlerRegistry

self._registry = RemoteButlerRegistry(self, self._connection)

self._registry_defaults = RegistryDefaults(
options.collections, options.run, options.inferDefaults, **options.kwargs
)
self._registry_defaults.finish(self._registry)
defaults = RegistryDefaults(options.collections, options.run, options.inferDefaults, **options.kwargs)
self._registry_defaults = DefaultsHolder(defaults)
self._registry = RemoteButlerRegistry(self, self._registry_defaults, self._connection)
defaults.finish(self._registry)

return self

Expand All @@ -164,16 +160,12 @@ def isWriteable(self) -> bool:
)
def collection_chains(self) -> ButlerCollections:
"""Object with methods for modifying collection chains."""
from ._registry import RemoteButlerRegistry

return RemoteButlerCollections(cast(RemoteButlerRegistry, self._registry), self._connection)
return self.collections

@property
def collections(self) -> ButlerCollections:
"""Object with methods for modifying and querying collections."""
from ._registry import RemoteButlerRegistry

return RemoteButlerCollections(cast(RemoteButlerRegistry, self._registry), self._connection)
return RemoteButlerCollections(self._registry_defaults, self._connection)

@property
def dimensions(self) -> DimensionUniverse:
Expand Down Expand Up @@ -563,7 +555,7 @@ def validateConfiguration(
@property
def run(self) -> str | None:
# Docstring inherited.
return self._registry_defaults.run
return self._registry_defaults.get().run

@property
def registry(self) -> Registry:
Expand Down Expand Up @@ -633,7 +625,7 @@ def _serialize_default_data_id(self) -> SerializedDataId:
# dimensions, but knowing what things are implied depends on what the
# required dimensions are.

return self._registry_defaults.dataId.to_simple(minimal=True).dataId
return self._registry_defaults.get().dataId.to_simple(minimal=True).dataId


def _to_file_payload(get_file_response: GetFileResponseModel) -> FileDatastoreGetPayload:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,39 +30,36 @@
__all__ = ("RemoteButlerCollections",)

from collections.abc import Iterable, Sequence, Set
from typing import TYPE_CHECKING

from lsst.utils.iteration import ensure_iterable

from .._butler_collections import ButlerCollections, CollectionInfo
from .._collection_type import CollectionType
from ..utils import has_globs
from ._collection_args import convert_collection_arg_to_glob_string_list
from ._defaults import DefaultsHolder
from ._http_connection import RemoteButlerHttpConnection, parse_model
from .server_models import QueryCollectionInfoRequestModel, QueryCollectionInfoResponseModel

if TYPE_CHECKING:
from ._registry import RemoteButlerRegistry


class RemoteButlerCollections(ButlerCollections):
"""Implementation of ButlerCollections for RemoteButler.
Parameters
----------
registry : `~lsst.daf.butler.registry.sql_registry.SqlRegistry`
Registry object used to work with the collections database.
defaults : `DefaultsHolder`
Registry object used to look up default collections.
connection : `RemoteButlerHttpConnection`
HTTP connection to Butler server.
"""

def __init__(self, registry: RemoteButlerRegistry, connection: RemoteButlerHttpConnection):
self._registry = registry
def __init__(self, defaults: DefaultsHolder, connection: RemoteButlerHttpConnection):
self._defaults = defaults
self._connection = connection

@property
def defaults(self) -> Sequence[str]:
return self._registry.defaults.collections
return self._defaults.get().collections

def extend_chain(self, parent_collection_name: str, child_collection_names: str | Iterable[str]) -> None:
raise NotImplementedError("Not yet available")
Expand Down

0 comments on commit f50f5cb

Please sign in to comment.