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

Switch abstract interfaces to protocols #120

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changes/120.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Refactor ``AbstractDataModel`` into a Python protocol as that is how it was
effectively used.
10 changes: 5 additions & 5 deletions docs/source/model_library.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ Model Library
=============

`~stpipe.library.AbstractModelLibrary` is a container designed to allow efficient processing of
collections of `~stpipe.datamodel.AbstractDataModel` instances created from an association.
collections of `~stpipe.protocols.DataModel` instances created from an association.

`~stpipe.library.AbstractModelLibrary` is an ordered collection (like a `list`) but provides:

Expand Down Expand Up @@ -234,8 +234,8 @@ allowing the `~stpipe.step.Step` to generate an :ref:`library_on_disk`

``Step.process`` can extend the above pattern to
support additional inputs (for example a single
`~stpipe.datamodel.AbstractDataModel` or filename containing
a `~stpipe.datamodel.AbstractDataModel`) to allow more
`~stpipe.protocols.DataModel` or filename containing
a `~stpipe.protocols.DataModel`) to allow more
flexible data processings, although some consideration
should be given to how to handle input that does not
contain association metadata. Does it make sense
Expand All @@ -252,9 +252,9 @@ Isolated Processing

Let's say we have a `~stpipe.step.Step`, ``flux_calibration``
that performs an operation that is only concerned with the data
for a single `~stpipe.datamodel.AbstractDataModel` at a time.
for a single `~stpipe.protocols.DataModel` at a time.
This step applies a function ``calibrate_model_flux`` that
accepts a single `~stpipe.datamodel.AbstractDataModel` and index as an input.
accepts a single `~stpipe.protocols.DataModel` and index as an input.
Its ``Step.process`` function can make good use of
`~stpipe.library.AbstractModelLibrary.map_function` to apply
this method to each model in the library.
Expand Down
6 changes: 3 additions & 3 deletions src/stpipe/config_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

from . import utilities
from .config import StepConfig
from .datamodel import AbstractDataModel
from .protocols import DataModel
from .utilities import _not_set

# Configure logger
Expand Down Expand Up @@ -82,7 +82,7 @@

def _is_datamodel(value):
"""Verify that value is a DataModel."""
if isinstance(value, AbstractDataModel):
if isinstance(value, DataModel):

Check warning on line 85 in src/stpipe/config_parser.py

View check run for this annotation

Codecov / codecov/patch

src/stpipe/config_parser.py#L85

Added line #L85 was not covered by tests
return value

raise VdtTypeError(value)
Expand All @@ -92,7 +92,7 @@
"""Verify that value is either a string (nominally a reference file path)
or a DataModel (possibly one with no corresponding file.)
"""
if isinstance(value, AbstractDataModel):
if isinstance(value, DataModel):
return value

if isinstance(value, str):
Expand Down
4 changes: 2 additions & 2 deletions src/stpipe/library.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

import asdf

from .datamodel import AbstractDataModel
from .protocols import DataModel

__all__ = [
"LibraryError",
Expand Down Expand Up @@ -771,7 +771,7 @@ def _model_to_filename(self, model):
return model_filename

def _to_group_id(self, model_or_filename, index):
if isinstance(model_or_filename, AbstractDataModel):
if isinstance(model_or_filename, DataModel):
getter = self._model_to_group_id
else:
getter = self._filename_to_group_id
Expand Down
2 changes: 1 addition & 1 deletion src/stpipe/pipeline.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None)
Either a class or instance of a class derived
from `Step`.

dataset : `stpipe.datamodel.AbstractDataModel`
dataset : `stpipe.protocols.DataModel`
A model of the input file. Metadata on this input file will
be used by the CRDS "bestref" algorithm to obtain a reference
file.
Expand Down
59 changes: 32 additions & 27 deletions src/stpipe/datamodel.py → src/stpipe/protocols.py
Original file line number Diff line number Diff line change
@@ -1,50 +1,54 @@
import abc
from __future__ import annotations

from abc import abstractmethod
from typing import TYPE_CHECKING, Protocol, runtime_checkable

class AbstractDataModel(abc.ABC):
if TYPE_CHECKING:
from collections.abc import Callable
from os import PathLike

Check warning on line 8 in src/stpipe/protocols.py

View check run for this annotation

Codecov / codecov/patch

src/stpipe/protocols.py#L7-L8

Added lines #L7 - L8 were not covered by tests


@runtime_checkable
class DataModel(Protocol):
"""
This Abstract Base Class is intended to cover multiple implementations of
data models so that each will be considered an appropriate subclass of this
class without requiring that they inherit this class.
This is a protocol to describe the methods and properties that define a
DataModel for the purposes of stpipe. This is a runtime checkable protocol
meaning that any object can be `isinstance` checked against this protocol
and will succeed even if the object does not inherit from this class.
Moreover, this object will act as an `abc.ABC` class if it is inherited from.

Any datamodel class instance that desires to be considered an instance of
AbstractDataModel must implement the following methods.
must fully implement the protocol in order to pass the `isinstance` check.

In addition, although it isn't yet checked (the best approach for supporting
this is still being considered), such instances must have a meta.filename
attribute.
"""

@classmethod
def __subclasshook__(cls, c_):
"""
Pseudo subclass check based on these attributes and methods
"""
if cls is AbstractDataModel:
mro = c_.__mro__
if (
any(hasattr(CC, "crds_observatory") for CC in mro)
and any(hasattr(CC, "get_crds_parameters") for CC in mro)
and any(hasattr(CC, "save") for CC in mro)
):
return True
return False

@property
@abc.abstractmethod
def crds_observatory(self):
@abstractmethod
def crds_observatory(self) -> str:
"""This should return a string identifying the observatory as CRDS expects it"""
...

Check warning on line 32 in src/stpipe/protocols.py

View check run for this annotation

Codecov / codecov/patch

src/stpipe/protocols.py#L32

Added line #L32 was not covered by tests

@abc.abstractmethod
def get_crds_parameters(self):
@property
@abstractmethod
def get_crds_parameters(self) -> dict[str, any]:
"""
This should return a dictionary of key/value pairs corresponding to the
parkey values CRDS is using to match reference files. Typically it returns
all metadata simple values.
"""
...

Check warning on line 42 in src/stpipe/protocols.py

View check run for this annotation

Codecov / codecov/patch

src/stpipe/protocols.py#L42

Added line #L42 was not covered by tests

@abc.abstractmethod
def save(self, path, dir_path=None, *args, **kwargs):
@abstractmethod
def save(
self,
path: PathLike | Callable[..., PathLike],
dir_path: PathLike | None = None,
*args,
**kwargs,
) -> PathLike:
"""
Save to a file.

Expand All @@ -64,3 +68,4 @@
output_path: str
The file path the model was saved in.
"""
...

Check warning on line 71 in src/stpipe/protocols.py

View check run for this annotation

Codecov / codecov/patch

src/stpipe/protocols.py#L71

Added line #L71 was not covered by tests
34 changes: 16 additions & 18 deletions src/stpipe/step.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@
DISCOURAGED_TYPES = None

from . import config, config_parser, crds_client, log, utilities
from .datamodel import AbstractDataModel
from .format_template import FormatTemplate
from .library import AbstractModelLibrary
from .protocols import DataModel
from .utilities import _not_set


Expand Down Expand Up @@ -398,7 +398,7 @@ def _check_args(self, args, discouraged_types, msg):
for i, arg in enumerate(args):
if isinstance(arg, discouraged_types):
self.log.error(
"%s %s object. Use an instance of AbstractDataModel instead.",
"%s %s object. Use an instance of DataModel instead.",
msg,
i,
)
Expand Down Expand Up @@ -492,7 +492,7 @@ def run(self, *args):
e,
)
library.shelve(model, i)
elif isinstance(args[0], AbstractDataModel):
elif isinstance(args[0], DataModel):
if self.class_alias is not None:
if isinstance(args[0], Sequence):
for model in args[0]:
Expand All @@ -506,7 +506,7 @@ def run(self, *args):
"header: %s",
e,
)
elif isinstance(args[0], AbstractDataModel):
elif isinstance(args[0], DataModel):
try:
args[0][
f"meta.cal_step.{self.class_alias}"
Expand Down Expand Up @@ -567,9 +567,7 @@ def run(self, *args):
for idx, result in enumerate(results_to_save):
if len(results_to_save) <= 1:
idx = None
if isinstance(
result, (AbstractDataModel | AbstractModelLibrary)
):
if isinstance(result, (DataModel | AbstractModelLibrary)):
self.save_model(result, idx=idx)
elif hasattr(result, "save"):
try:
Expand Down Expand Up @@ -609,7 +607,7 @@ def finalize_result(self, result, reference_files_used):

Parameters
----------
result : a datamodel that is an instance of AbstractDataModel or
result : a datamodel that is an instance of DataModel or
collections.abc.Sequence
One step result (potentially of many).

Expand Down Expand Up @@ -780,7 +778,7 @@ def get_ref_override(self, reference_file_type):
"""
override_name = crds_client.get_override_name(reference_file_type)
path = getattr(self, override_name, None)
if isinstance(path, AbstractDataModel):
if isinstance(path, DataModel):
return path

return abspath(path) if path and path != "N/A" else path
Expand All @@ -795,7 +793,7 @@ def get_reference_file(self, input_file, reference_file_type):

Parameters
----------
input_file : a datamodel that is an instance of AbstractDataModel
input_file : a datamodel that is an instance of DataModel
A model of the input file. Metadata on this input file
will be used by the CRDS "bestref" algorithm to obtain a
reference file.
Expand All @@ -810,7 +808,7 @@ def get_reference_file(self, input_file, reference_file_type):
"""
override = self.get_ref_override(reference_file_type)
if override is not None:
if isinstance(override, AbstractDataModel):
if isinstance(override, DataModel):
self._reference_files_used.append(
(reference_file_type, override.override_handle)
)
Expand Down Expand Up @@ -846,7 +844,7 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None)
cls : stpipe.Step
Either a class or instance of a class derived
from `Step`.
dataset : A datamodel that is an instance of AbstractDataModel
dataset : A datamodel that is an instance of DataModel
A model of the input file. Metadata on this input file will
be used by the CRDS "bestref" algorithm to obtain a reference
file.
Expand Down Expand Up @@ -874,7 +872,7 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None)
if crds_observatory is None:
raise ValueError("Need a valid name for crds_observatory.")
else:
# If the dataset is not an operable instance of AbstractDataModel,
# If the dataset is not an operable instance of DataModel,
# log as such and return an empty config object
try:
with cls._datamodels_open(dataset, asn_n_members=1) as model:
Expand All @@ -884,7 +882,7 @@ def get_config_from_reference(cls, dataset, disable=None, crds_observatory=None)
crds_parameters = model.get_crds_parameters()
crds_observatory = model.crds_observatory
except (OSError, TypeError, ValueError):
logger.warning("Input dataset is not an instance of AbstractDataModel.")
logger.warning("Input dataset is not an instance of DataModel.")
disable = True

# Check if retrieval should be attempted.
Expand Down Expand Up @@ -930,7 +928,7 @@ def set_primary_input(self, obj, exclusive=True):

Parameters
----------
obj : str, pathlib.Path, or instance of AbstractDataModel
obj : str, pathlib.Path, or instance of DataModel
The object to base the name on. If a datamodel,
use Datamodel.meta.filename.

Expand All @@ -945,7 +943,7 @@ def set_primary_input(self, obj, exclusive=True):
if not exclusive or parent_input_filename is None:
if isinstance(obj, str | Path):
self._input_filename = str(obj)
elif isinstance(obj, AbstractDataModel):
elif isinstance(obj, DataModel):
try:
self._input_filename = obj.meta.filename
except AttributeError:
Expand All @@ -967,7 +965,7 @@ def save_model(

Parameters
----------
model : a instance of AbstractDataModel
model : a instance of DataModel
The model to save.

suffix : str
Expand Down Expand Up @@ -1166,7 +1164,7 @@ def open_model(self, init, **kwargs):

Returns
-------
datamodel : instance of AbstractDataModel
datamodel : instance of DataModel
Object opened as a datamodel
"""
# Use the parent method if available, since this step
Expand Down
4 changes: 2 additions & 2 deletions src/stpipe/subproc.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import os
import subprocess

from .datamodel import AbstractDataModel
from .protocols import DataModel
from .step import Step


Expand All @@ -28,7 +28,7 @@ class SystemCall(Step):
def process(self, *args):
newargs = []
for i, arg in enumerate(args):
if isinstance(arg, AbstractDataModel):
if isinstance(arg, DataModel):
filename = f"{self.qualified_name}.{i:04d}.{self.output_ext}"
arg.save(filename)
newargs.append(filename)
Expand Down
57 changes: 0 additions & 57 deletions tests/test_abstract_datamodel.py

This file was deleted.

Loading