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

🐛 FIX: aiida/repository typing #4920

Merged
merged 19 commits into from
May 7, 2021
Merged
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
1 change: 1 addition & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ repos:
aiida/manage/database/delete/nodes.py|
aiida/orm/nodes/node.py|
aiida/orm/nodes/process/.*py|
aiida/repository/.*py|
aiida/tools/graph/graph_traversers.py|
aiida/tools/groups/paths.py|
aiida/tools/importexport/archive/.*py|
Expand Down
2 changes: 1 addition & 1 deletion aiida/backends/general/migrations/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def is_initialised(self) -> bool:
def erase(self):
raise NotImplementedError()

def put_object_from_filelike(self, handle: io.BufferedIOBase) -> str:
def _put_object_from_filelike(self, handle: io.BufferedIOBase) -> str:
"""Store the byte contents of a file in the repository.

:param handle: filelike object with the byte content to be stored.
Expand Down
2 changes: 1 addition & 1 deletion aiida/orm/nodes/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ def list_object_names(self, path: str = None) -> typing.List[str]:
return self._repository.list_object_names(path)

@contextlib.contextmanager
def open(self, path: str, mode='r') -> io.BufferedReader:
def open(self, path: str, mode='r') -> typing.Iterator[typing.BinaryIO]:
"""Open a file handle to an object stored under the given key.

.. note:: this should only be used to open a handle to read an existing file. To write a new file use the method
Expand Down
2 changes: 1 addition & 1 deletion aiida/repository/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@
from .common import *
from .repository import *

__all__ = (backend.__all__ + common.__all__ + repository.__all__)
__all__ = (backend.__all__ + common.__all__ + repository.__all__) # type: ignore[name-defined]
2 changes: 1 addition & 1 deletion aiida/repository/backend/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@
from .disk_object_store import *
from .sandbox import *

__all__ = (abstract.__all__ + disk_object_store.__all__ + sandbox.__all__)
__all__ = (abstract.__all__ + disk_object_store.__all__ + sandbox.__all__) # type: ignore[name-defined]
14 changes: 10 additions & 4 deletions aiida/repository/backend/abstract.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def is_initialised(self) -> bool:
"""Return whether the repository has been initialised."""

@abc.abstractmethod
def erase(self):
def erase(self) -> None:
"""Delete the repository itself and all its contents.

.. note:: This should not merely delete the contents of the repository but any resources it created. For
Expand All @@ -53,17 +53,23 @@ def erase(self):
"""

@staticmethod
def is_readable_byte_stream(handle):
def is_readable_byte_stream(handle) -> bool:
return hasattr(handle, 'read') and hasattr(handle, 'mode') and 'b' in handle.mode

def put_object_from_filelike(self, handle: io.BufferedIOBase) -> str:
def put_object_from_filelike(self, handle: typing.BinaryIO) -> str:
"""Store the byte contents of a file in the repository.

:param handle: filelike object with the byte content to be stored.
:return: the generated fully qualified identifier for the object within the repository.
:raises TypeError: if the handle is not a byte stream.
"""
if not isinstance(handle, io.BytesIO) and not self.is_readable_byte_stream(handle):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy didn't like this, and it feels like it goes a bit against the concept of an abstract method, so I moved it to a separate method. But its not a deal breaker

Copy link
Member Author

@chrisjsewell chrisjsewell May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the best way to do this technically is something like:

    def put_object_from_filelike(self, handle: typing.BinaryIO) -> str:
        """Store the byte contents of a file in the repository.
        :param handle: filelike object with the byte content to be stored.
        :return: the generated fully qualified identifier for the object within the repository.
        """
		check_byte_stream(handle)
        return _put_object_from_filelike(handle)

    @abc.abstractmethod
    def _put_object_from_filelike(self, handle: typing.BinaryIO) -> str:

then the concrete methods do not have to "remember" to call super() or any other initial code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

raise TypeError(f'handle does not seem to be a byte stream: {type(handle)}.')
return self._put_object_from_filelike(handle)

@abc.abstractmethod
def _put_object_from_filelike(self, handle: typing.BinaryIO) -> str:
pass

def put_object_from_file(self, filepath: typing.Union[str, pathlib.Path]) -> str:
"""Store a new object with contents of the file located at `filepath` on this file system.
Expand All @@ -84,7 +90,7 @@ def has_object(self, key: str) -> bool:
"""

@contextlib.contextmanager
def open(self, key: str) -> io.BufferedIOBase:
def open(self, key: str) -> typing.Iterator[typing.BinaryIO]: # type: ignore[return]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, these abstract methods, that you also need to call super() on, mypy is not a fan 😬

"""Open a file handle to an object stored under the given key.

.. note:: this should only be used to open a handle to read an existing file. To write a new file use the method
Expand Down
12 changes: 6 additions & 6 deletions aiida/repository/backend/disk_object_store.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# -*- coding: utf-8 -*-
"""Implementation of the ``AbstractRepositoryBackend`` using the ``disk-objectstore`` as the backend."""
import contextlib
import io
import shutil
import typing

Expand All @@ -23,7 +22,9 @@ def __init__(self, container):

def __str__(self) -> str:
"""Return the string representation of this repository."""
return f'DiskObjectStoreRepository: {self.container.container_id} | {self.container.get_folder()}'
if self.is_initialised:
return f'DiskObjectStoreRepository: {self.container.container_id} | {self.container.get_folder()}'
return 'DiskObjectStoreRepository: <uninitialised>'

@property
def uuid(self) -> typing.Optional[str]:
Expand All @@ -45,7 +46,7 @@ def is_initialised(self) -> bool:
return self.container.is_initialised

@property
def container(self):
def container(self) -> Container:
return self._container

def erase(self):
Expand All @@ -55,14 +56,13 @@ def erase(self):
except FileNotFoundError:
pass

def put_object_from_filelike(self, handle: io.BufferedIOBase) -> str:
def _put_object_from_filelike(self, handle: typing.BinaryIO) -> str:
"""Store the byte contents of a file in the repository.

:param handle: filelike object with the byte content to be stored.
:return: the generated fully qualified identifier for the object within the repository.
:raises TypeError: if the handle is not a byte stream.
"""
super().put_object_from_filelike(handle)
return self.container.add_object(handle.read())

def has_object(self, key: str) -> bool:
Expand All @@ -74,7 +74,7 @@ def has_object(self, key: str) -> bool:
return self.container.has_object(key)

@contextlib.contextmanager
def open(self, key: str) -> io.BufferedIOBase:
def open(self, key: str) -> typing.Iterator[typing.BinaryIO]:
"""Open a file handle to an object stored under the given key.

.. note:: this should only be used to open a handle to read an existing file. To write a new file use the method
Expand Down
14 changes: 7 additions & 7 deletions aiida/repository/backend/sandbox.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# -*- coding: utf-8 -*-
"""Implementation of the ``AbstractRepositoryBackend`` using a sandbox folder on disk as the backend."""
import contextlib
import io
import os
import shutil
import typing
Expand All @@ -16,11 +15,14 @@ class SandboxRepositoryBackend(AbstractRepositoryBackend):
"""Implementation of the ``AbstractRepositoryBackend`` using a sandbox folder on disk as the backend."""

def __init__(self):
self._sandbox = None
from aiida.common.folders import SandboxFolder
self._sandbox: typing.Optional[SandboxFolder] = None

def __str__(self) -> str:
"""Return the string representation of this repository."""
return f'SandboxRepository: {self._sandbox.abspath}'
if self.is_initialised:
return f'SandboxRepository: {self._sandbox.abspath if self._sandbox else "null"}'
sphuber marked this conversation as resolved.
Show resolved Hide resolved
return 'SandboxRepository: <uninitialised>'

def __del__(self):
"""Delete the entire sandbox folder if it was instantiated and still exists."""
Expand Down Expand Up @@ -68,15 +70,13 @@ def erase(self):
finally:
self._sandbox = None

def put_object_from_filelike(self, handle: io.BufferedIOBase) -> str:
def _put_object_from_filelike(self, handle: typing.BinaryIO) -> str:
"""Store the byte contents of a file in the repository.

:param handle: filelike object with the byte content to be stored.
:return: the generated fully qualified identifier for the object within the repository.
:raises TypeError: if the handle is not a byte stream.
"""
super().put_object_from_filelike(handle)

key = str(uuid.uuid4())
filepath = os.path.join(self.sandbox.abspath, key)

Expand All @@ -94,7 +94,7 @@ def has_object(self, key: str) -> bool:
return key in os.listdir(self.sandbox.abspath)

@contextlib.contextmanager
def open(self, key: str) -> io.BufferedIOBase:
def open(self, key: str) -> typing.Iterator[typing.BinaryIO]:
"""Open a file handle to an object stored under the given key.

.. note:: this should only be used to open a handle to read an existing file. To write a new file use the method
Expand Down
Loading