Implement a FSspec based ArtifactLoader #100

maxmynter · 2024-03-07T15:29:16Z

Closes #94

Questions

Should fsspec become a first class dependecy such as tabulate? Or do we import it conditionally in the FilepathArtifactLoader?

nicholasjng · 2024-03-08T13:59:21Z

src/nnbench/types.py

+        self, path: str | os.PathLike[str], to_localdir: str | os.PathLike[str] | None = None
+    ) -> None:
+        self.rpath = str(path)
+        self.lpath = Path(to_localdir or mkdtemp())


This is an acquired/created resource that needs proper cleanup, e.g. through a weakref finalizer (see e.g. https://github.com/python/cpython/blob/2647afeab694517933c53028d4ad427460b9e1fa/Lib/tempfile.py#L885-L888)

nicholasjng · 2024-03-08T14:01:09Z

src/nnbench/types.py

+        Path
+            The path to the artifact on the local filesystem.
+        """
+        fs = spec.AbstractFileSystem()


The filesystem should be constructed directly from the URI, see e.g. https://github.com/nicholasjng/shelf/blob/4a628c17e678524a49cb327238c3fb7924e0cb9d/src/shelf/util.py#L12.

nicholasjng

What is the story after I downloaded the remote files to my machine? I mean, what do I have to use to instantiate my model from the files?

nicholasjng · 2024-03-08T17:12:57Z

pyproject.toml

@@ -31,7 +31,7 @@ classifiers = [
    "Typing :: Typed",
 ]

-dependencies = ["tabulate"]
+dependencies = ["tabulate", "fsspec"]


In my opinion, we should make this optional (i.e. by importing fsspec only locally in the artifact load method), and facilitate local loading with builtin methods.

nicholasjng · 2024-03-08T17:14:55Z

src/nnbench/types.py

 from typing import Any, Callable, Generic, Iterable, Iterator, Literal, TypeVar

+from fsspec import core, filesystem, utils


Please import the specific APIs from these submodules, not the whole modules themselves.

nicholasjng · 2024-03-08T17:16:24Z

tests/test_artifacts.py

+
+def test_load_local_file(local_file: Path, tmp_path: Path) -> None:
+    test_dir = tmp_path / "test_load_dir"
+    loader = FilePathArtifactLoader(str(local_file), str(test_dir))


This should work without the string casts.

nicholasjng · 2024-03-08T17:18:36Z

src/nnbench/types.py

+    def __init__(
+        self,
+        path: str | os.PathLike[str],
+        to_localdir: str | os.PathLike[str] | None = None,


This should have a different name, it currently sounds like a bool. Maybe destination / basedir ?

nicholasjng · 2024-03-08T17:19:43Z

src/nnbench/types.py

+        fs, _, _ = core.get_fs_token_paths(self.source_path)
+        if isinstance(fs, LocalFileSystem):
+            return Path(self.source_path).resolve()


Is this caching a la "check if it exists, only download if it doesn't"? (Otherwise it's probably currently missing)

More or less.

My understanding is, that this class, as a general filesystem class, should also be able to use local filepaths.
In that case, we don't want to download the data.

If it is a remote path, we download and put it either (a) a specified local dir, or (b) a tmp dir.

If a) the files are persisted on disk and the filepath can be used (by the user) in other places.

If b) the cleanup task deletes the tmp dir and the model to not clutter the filesystem.

That is how it is currently implemented. Do you agree with this?

nicholasjng · 2024-03-11T10:08:54Z

.github/workflows/lint-and-test.yaml

@@ -58,6 +58,7 @@ jobs:
    - name: Run tests on oldest supported Python
      run: |
        python -m pip install ".[dev]"
+        pip install fsspec


No, just add it to the dev extra.

Please remove this line, then.

src/nnbench/types.py

nicholasjng · 2024-03-11T10:13:29Z

src/nnbench/types.py

+            raise ImportError(
+                "The FilePathArtifactLoader depends on fsspec. Do you have it installed?"
+            )


That's not helpful, you should instead give an install instruction:

Suggested change

raise ImportError(

"The FilePathArtifactLoader depends on fsspec. Do you have it installed?"

)

raise ImportError(

"class {self.__class__.__name__} requires `fsspec` to be installed. You can install it by running `python -m pip install --upgrade fsspec`"

)

nicholasjng · 2024-03-11T10:15:30Z

src/nnbench/types.py

+        Path
+            The path to the artifact on the local filesystem.
+        """
+        fs, _, _ = self._get_fs_token_paths(self.source_path)


Since you're ignoring the other two outputs, you could also use fs = fsspec.filesystem(self.source_path, **self.storage_options) (or with the options in another set of parentheses, not sure about the calling convention).

nicholasjng · 2024-03-11T10:16:40Z

src/nnbench/types.py

+        """
+        fs, _, _ = self._get_fs_token_paths(self.source_path)
+        if isinstance(fs, self._LocalFileSystem):
+            return Path(self.source_path).resolve()


You might want to copy files into your target folder even if they are local? Seems like you could just omit the branch here.

nicholasjng · 2024-03-11T14:48:26Z

src/nnbench/types.py

+    def _get_protocol(self, path: str) -> str:
+        """Lazy loading of fsspec.utils.get_protocol"""
+        try:
+            from fsspec.utils import get_protocol
+
+            return get_protocol(path)
+        except ImportError:
+            raise ImportError(
+                "class {self.__class__.__name__} requires `fsspec` to be installed. You can install it by running `python -m pip install --upgrade fsspec`"
+            )


Two things here:

The single use in load() below does not warrant extracting these into their own APIs - please just import the fsspec versions both inline in load(), and scrap the private class methods.

You can factor out the import error with a simple import guard at the top of the file, e.g. like so:

# this towards the top, after the imports. try: import fsspec HAS_FSSPEC = True except ImportError: HAS_FSSPEC = False ... # later, in FilePathArtifactLoader.__init__(): ... if not HAS_FSSPEC: raise ModuleNotFoundError("class blah requires `fsspec`...")

nicholasjng · 2024-03-11T14:48:55Z

.github/workflows/lint-and-test.yaml

@@ -58,6 +58,7 @@ jobs:
    - name: Run tests on oldest supported Python
      run: |
        python -m pip install ".[dev]"
+        pip install fsspec


Please remove this line, then.

nicholasjng · 2024-03-11T14:50:28Z

src/nnbench/types.py

+if TYPE_CHECKING:
+    import fsspec


Is this mypy complaining? You could also add fsspec to the list of modules for which to ignore missing imports (in the pyproject.toml) if you like, since most of its interfaces are not great typing-wise.

Yup, was mypy. Changed to ignore in the .toml.

maxmynter requested a review from nicholasjng March 7, 2024 15:36

maxmynter force-pushed the fsspec-loader branch from 6a0570e to 4dea5be Compare March 8, 2024 13:56

nicholasjng reviewed Mar 8, 2024

View reviewed changes

maxmynter added 2 commits March 8, 2024 15:30

Add fsspec ArtifactLoader

facbc71

Implement Reviewers Feedback

64777cd

maxmynter force-pushed the fsspec-loader branch from 4dea5be to 64777cd Compare March 8, 2024 15:52

Add fsspec as dependency

36730c0

maxmynter requested a review from nicholasjng March 8, 2024 15:58

maxmynter marked this pull request as ready for review March 8, 2024 15:58

nicholasjng reviewed Mar 8, 2024

View reviewed changes

maxmynter added 3 commits March 11, 2024 10:32

Review Comments

3157354

Docstring update

3f26bd9

Install fsspec in test environment

ce7f0dd

maxmynter force-pushed the fsspec-loader branch from 9c53388 to ce7f0dd Compare March 11, 2024 09:46

maxmynter requested a review from nicholasjng March 11, 2024 09:46

nicholasjng reviewed Mar 11, 2024

View reviewed changes

Reviewer comments pt.2

35141f4

maxmynter requested a review from nicholasjng March 11, 2024 14:26

nicholasjng reviewed Mar 11, 2024

View reviewed changes

Review v3

ed83dd9

maxmynter requested a review from nicholasjng March 11, 2024 15:20

nicholasjng approved these changes Mar 11, 2024

View reviewed changes

maxmynter merged commit 972bff8 into main Mar 11, 2024
5 checks passed

maxmynter deleted the fsspec-loader branch March 11, 2024 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a FSspec based ArtifactLoader #100

Implement a FSspec based ArtifactLoader #100

maxmynter commented Mar 7, 2024 •

edited

Loading

nicholasjng Mar 8, 2024

nicholasjng Mar 8, 2024

nicholasjng left a comment

nicholasjng Mar 8, 2024

maxmynter Mar 8, 2024

nicholasjng Mar 8, 2024

nicholasjng Mar 8, 2024

nicholasjng Mar 8, 2024

nicholasjng Mar 8, 2024

maxmynter Mar 8, 2024

nicholasjng Mar 11, 2024

nicholasjng Mar 11, 2024

nicholasjng Mar 11, 2024

nicholasjng Mar 11, 2024

nicholasjng Mar 11, 2024

nicholasjng Mar 11, 2024

nicholasjng Mar 11, 2024

nicholasjng Mar 11, 2024

maxmynter Mar 11, 2024

		from typing import Any, Callable, Generic, Iterable, Iterator, Literal, TypeVar

		from fsspec import core, filesystem, utils

Implement a FSspec based ArtifactLoader #100

Implement a FSspec based ArtifactLoader #100

Conversation

maxmynter commented Mar 7, 2024 • edited Loading

Questions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholasjng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxmynter commented Mar 7, 2024 •

edited

Loading