Add audbackend.backend.Minio #231

hagenw · 2024-09-03T15:00:19Z

Implements a backend for MinIO.

As MinIO is compatible with S3 storage, this might already work with other S3 storage as well. @ChristianGeng has tested this partly for a S3 instance at Hetzner, which worked with the exception that creation and deletion of buckets and empty files were not supported.

I decided to handle the authentication the same way as we do for audbackend.backend.Artifactory, by allowing to specify the username / password inside a config file or environment variable. In addition, we have the class method audbackend.backend.Minio.get_authentication() to retrieve them, in order to check their values.
For local MinIO servers we also need to set the secure argument to False. I added this to the config file as well, in oder to make it part of the backend configuration, and added the class method audbackend.backend.Minio.get_config(), which returns all entries as dictionary.
In certain edge cases you might want to add other arguments to the underlying minio.Minio class. For that reason I decided to add support for **kwargs, but do no checks if there is any conflict with authentication as I think this is only for power users anyway.

Summary by Sourcery

Add support for MinIO as a new backend in the audbackend library, enabling storage operations with MinIO. Refactor tests to accommodate the new backend and ensure robust testing of its functionality.

New Features:

Introduce a new backend for MinIO, allowing integration with MinIO storage systems.

Enhancements:

Refactor test parameterization to use a centralized list of backend-interface combinations, improving test maintainability.

Tests:

Add comprehensive tests for the new MinIO backend, including authentication, file operations, and configuration parsing.

codecov · 2024-09-04T11:33:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.0%. Comparing base (937da89) to head (af109fc).

Additional details and impacted files

Files with missing lines	Coverage Δ
audbackend/backend/__init__.py	`100.0% <100.0%> (ø)`
audbackend/core/api.py	`100.0% <100.0%> (ø)`
audbackend/core/backend/minio.py	`100.0% <100.0%> (ø)`

ChristianGeng · 2024-10-18T16:52:43Z

audbackend/core/backend/minio.py

+
+        """
+        path = path.replace(self.sep, "/")
+        if path.startswith("/"):


In Python versions greater than 3.8 it would be safe to remove the conditional and replace it with. path = path.removeprefix("/"). I believe that it is getting close to deprecating 3.8?

When removing Python 3.8, there are other things we should update, e.g. if I remember correctly the typing module might no longer be needed. I would prefer to first create a general issue to gather all the things we would like to update when removing Python 3.8, and then do this in dedicated pull requests.

Which means for now in this pull request we need to still support Python 3.8.

This is now tracked in [tools/meta#12] internally.

audbackend/core/backend/minio.py

ChristianGeng · 2024-10-18T17:17:40Z

audbackend/core/backend/minio.py

+        cannot be closed.
+
+        """
+        # At the moment, this is automatically handled.


Automatically handled means by the minio client lib. Would it make sense to make this explicit?

The docstring is identical to the one in the base class Base. Possibly the information that this is automatically handled should go into the subclass docstring?

I removed _close() and added close() with an updated docstring instead:

In principle, we can also take care about opening and closing by providing an http_client object to MinIO, but as it will be very complicated to find out the best settings in that case, I would stay for now with the default and just state that closing is automatically handled by MinIO.

ChristianGeng · 2024-10-18T17:28:14Z

audbackend/core/backend/minio.py

+        r"""Copy file on backend."""
+        src_path = self.path(src_path)
+        dst_path = self.path(dst_path)
+        # `copy_object()` has a maximum size limit of 5GB.


The 5GB limit is a limit of S3 native? This post suggests this at least. So the minio client uses s3n?

Don't know, I just found the information in the MinIO documentation. As we don't have a test for this, maybe we should also lower the value slightly to be on the safe side?

Probably lowering would be ok to get it working right now.
The background is a different one: if MinIO has the limit of 5GB this suggests that the minio package is using the old s3n interface. Afaiu s3a succeeds s3n. The difference between is that s3n supports objects up to 5GB, but s3a should support 5TB and has improved performance. I cannof find the link right now, but I think I had seen that fsspec has an implementation of s3a.

I changed the size limit to 4.9 GB to be on the safe side.

* Add support for owner() * Be more conservative regarding owner

ChristianGeng · 2024-10-18T19:22:38Z

audbackend/core/backend/minio.py

+    def _size(
+        self,
+        path: str,
+    ) -> str:


This is used by _copy_file where it is used to determine whether file size exceeds 5GB. However I can see no cast in _copy_file. Should the return type hint be integer?

Good catch, it returns indeed an integer. I corrected the typing information.

ChristianGeng · 2024-10-21T06:49:15Z

tests/test_backend_minio.py

+        backend.open()
+
+
+def test_get_config(tmpdir, hosts, hide_credentials):


This tests parsing of the configuration, but does not have a one-line docstring.

I added a docstring.

ChristianGeng · 2024-10-21T06:51:35Z

tests/test_backend_minio.py

+)
+def test_maven_file_structure(
+    tmpdir, interface, file, version, extensions, regex, expected
+):


Although the test name is already telling, a single line docstring would be nice.

I added a docstring.

ChristianGeng · 2024-10-21T09:21:04Z

tests/test_interface_unversioned.py

@@ -91,6 +91,7 @@ def tree(tmpdir, request):
    [
        (audbackend.backend.Artifactory, audbackend.interface.Unversioned),
        (audbackend.backend.FileSystem, audbackend.interface.Unversioned),
+        (audbackend.backend.Minio, audbackend.interface.Unversioned),


The interface parametrization seems to vary little between tests.

Would it make sense to recast this into a fixture? Possibly like this:

@pytest.fixture(params=[ (audbackend.backend.Artifactory, audbackend.interface.Unversioned), (audbackend.backend.FileSystem, audbackend.interface.Unversioned), (audbackend.backend.Minio, audbackend.interface.Unversioned), (SingleFolder, audbackend.interface.Unversioned), ]) def interface(request): return request.param

Probably local to test_interface_unversioned.py would be sufficient.

interface was already a fixture, but I could solve the problem by a variable:

# Backend-interface combinations to use in all tests backend_interface_combinations = [ (audbackend.backend.Artifactory, audbackend.interface.Unversioned), (audbackend.backend.FileSystem, audbackend.interface.Unversioned), (audbackend.backend.Minio, audbackend.interface.Unversioned), (SingleFolder, audbackend.interface.Unversioned), ] # ... @pytest.mark.parametrize( "interface", backend_interface_combinations, indirect=True, ) def test_archive(tmpdir, tree, archive, files, tmp_root, interface, expected):

sourcery-ai · 2024-10-22T12:42:48Z

Reviewer's Guide by Sourcery

This pull request implements a new backend for MinIO in the audbackend library. It adds support for MinIO storage, which is compatible with S3, and includes necessary changes to the existing codebase to integrate the new backend. The implementation follows a similar pattern to the existing Artifactory backend, with additional configuration options specific to MinIO.

Class diagram for the new Minio backend

classDiagram
    class Minio {
        - MinioClient _client
        + Minio(host: str, repository: str, authentication: Tuple[str, str] = None, secure: bool = None, **kwargs)
        + get_authentication(host: str) Tuple[str, str]
        + get_config(host: str) Dict
        + close()
        + _checksum(path: str) str
        + _collapse(path)
        + _copy_file(src_path: str, dst_path: str, verbose: bool)
        + _create()
        + _date(path: str) str
        + _delete()
        + _exists(path: str) bool
        + _get_file(src_path: str, dst_path: str, verbose: bool)
        + _ls(path: str) List[str]
        + _move_file(src_path: str, dst_path: str, verbose: bool)
        + _open()
        + _owner(path: str) str
        + path(path: str) str
        + _put_file(src_path: str, dst_path: str, checksum: str, verbose: bool)
        + _remove_file(path: str)
        + _size(path: str) int
    }
    class Base {
        <<abstract>>
    }
    Minio --|> Base
    note for Minio "This class implements a backend for MinIO storage, compatible with S3."

File-Level Changes

Change	Details	Files
Implement MinIO backend class	Create new Minio class inheriting from Base Implement authentication and configuration methods Add methods for file operations (put, get, copy, move, delete) Handle large file transfers (>5GB) with a fallback mechanism	`audbackend/core/backend/minio.py`
Integrate MinIO backend into existing codebase	Register MinIO backend in core API Add MinIO import in backend init.py Update test configurations to include MinIO Modify existing tests to accommodate MinIO backend	`audbackend/core/api.py` `audbackend/backend/__init__.py` `tests/conftest.py` `tests/test_interface_unversioned.py` `tests/test_interface_versioned.py` `tests/test_interface_maven.py`
Add new tests for MinIO backend	Create test_backend_minio.py with comprehensive tests Test authentication, configuration, and file operations Add tests for content type handling and large file copying	`tests/test_backend_minio.py`
Update existing tests to include MinIO	Modify test_api.py to include MinIO in backend tests Update test_interface_unversioned.py, test_interface_versioned.py, and test_interface_maven.py to include MinIO in interface tests	`tests/test_api.py` `tests/test_interface_unversioned.py` `tests/test_interface_versioned.py` `tests/test_interface_maven.py`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @hagenw - I've reviewed your changes and found some issues that need to be addressed.

Blocking issues:

Hardcoded MinIO access key found. (link)
Hardcoded MinIO secret key found. (link)

Here's what I looked at during the review

🟡 General issues: 1 issue found
🔴 Security: 2 blocking issues
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

audbackend/core/backend/minio.py

tests/conftest.py

audbackend/core/backend/minio.py

tests/conftest.py

tests/test_backend_minio.py

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

hagenw · 2024-10-22T14:26:48Z

I addressed now all comments and updated the description by mentioning our Hetzner S3 test.

Add audbackend.backend.MinIO

e7bbfa0

hagenw marked this pull request as draft September 3, 2024 15:00

hagenw added 9 commits September 3, 2024 17:01

Add dependency to minio

cf34071

Rename MinIO to Minio

dd642d0

Fix Minio.delete() to first remove objects

40a2654

Let Minio.exists() return True for "/"

64941c5

Fix Minio.ls() for non-existing paths

fe4b88d

Add tests using a local MinIO server

054174e

Expand tests to 100% code coverage

d19d390

Fix remaining issues

07b34df

Update docstrings

3ed2685

hagenw changed the title ~~Add audbackend.backend.MinIO~~ Add audbackend.backend.Minio Sep 4, 2024

hagenw added 2 commits September 4, 2024 12:55

Define constant hosts values at single position

77b3a14

Run tests on play.min.io

3a8517d

hagenw added 8 commits September 4, 2024 14:07

Be more conservative for other copy method

7073080

Add Minio.get_config()

c5e6a27

Add docstring example

393cff7

Correct get_config() docstring

2284cfc

Add secure argument

9faf954

Add possibility to provide **kwargs

1005c6b

Set content-type during upload

92282f6

Add **kwargs to docstring

c70ae7e

hagenw mentioned this pull request Oct 1, 2024

Add support for owner() #234

Merged

ChristianGeng reviewed Oct 18, 2024

View reviewed changes

audbackend/core/backend/minio.py Show resolved Hide resolved

ChristianGeng reviewed Oct 18, 2024

View reviewed changes

hagenw added 2 commits October 18, 2024 19:41

Add support for owner() (#234)

371cc8c

* Add support for owner() * Be more conservative regarding owner

Fix owner test under Windows

643c85d

ChristianGeng reviewed Oct 18, 2024

View reviewed changes

ChristianGeng reviewed Oct 21, 2024

View reviewed changes

hagenw mentioned this pull request Oct 21, 2024

Evaluate influence of chunk size on downloads #235

Open

ChristianGeng reviewed Oct 21, 2024

View reviewed changes

Try to fix Windows owner test

74f5c8f

hagenw force-pushed the minio branch from 29b5990 to 74f5c8f Compare October 22, 2024 09:23

hagenw added 11 commits October 22, 2024 11:33

Replace _close() with close()

792fd5a

Limit copy size to 4.9 GB

6901166

Ensure errors are tested for MinIO

520c129

Add docstring to test_get_config

86aaf4c

Add docstring to test_maven_file_structure

2a92642

Ground truth var for interfaces in _unversioned.py

52a9597

Ground truth var for interfaces in _versioned.py

fc86e53

Ground truth var for interfaces in _maven.py

cb67cd3

Fix typos

3aabdf0

Update get_file test

a044205

Fix typing of _size()

4f65f5f

hagenw marked this pull request as ready for review October 22, 2024 12:41

sourcery-ai bot reviewed Oct 22, 2024

View reviewed changes

hagenw and others added 8 commits October 22, 2024 14:54

Remove default None in dict.get()

ccf7d63

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

Improve code quality

e131cba

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>

Add comment for MinIO credentials

6292bfc

Improve code quality

785305a

Add exception in _get_file()

3f0a4b0

Fix coverage

601a825

DEBUG: coverage

60c9479

Try to fix coverage

af109fc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add audbackend.backend.Minio #231

Add audbackend.backend.Minio #231

hagenw commented Sep 3, 2024 •

edited

Loading

codecov bot commented Sep 4, 2024 •

edited

Loading

ChristianGeng Oct 18, 2024

hagenw Oct 18, 2024

hagenw Oct 22, 2024

ChristianGeng Oct 18, 2024 •

edited

Loading

hagenw Oct 22, 2024

hagenw Oct 22, 2024

ChristianGeng Oct 18, 2024

hagenw Oct 18, 2024

ChristianGeng Oct 21, 2024

hagenw Oct 22, 2024

ChristianGeng Oct 18, 2024

hagenw Oct 22, 2024

ChristianGeng Oct 21, 2024

hagenw Oct 22, 2024

ChristianGeng Oct 21, 2024

hagenw Oct 22, 2024

ChristianGeng Oct 21, 2024 •

edited

Loading

hagenw Oct 22, 2024

sourcery-ai bot commented Oct 22, 2024

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

hagenw commented Oct 22, 2024

		backend.open()


		def test_get_config(tmpdir, hosts, hide_credentials):

Add audbackend.backend.Minio #231

Are you sure you want to change the base?

Add audbackend.backend.Minio #231

Conversation

hagenw commented Sep 3, 2024 • edited Loading

Summary by Sourcery

codecov bot commented Sep 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianGeng Oct 18, 2024 • edited Loading

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

ChristianGeng Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourcery-ai bot commented Oct 22, 2024

Reviewer's Guide by Sourcery

Class diagram for the new Minio backend

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

Choose a reason for hiding this comment

hagenw commented Oct 22, 2024

hagenw commented Sep 3, 2024 •

edited

Loading

codecov bot commented Sep 4, 2024 •

edited

Loading

ChristianGeng Oct 18, 2024 •

edited

Loading

ChristianGeng Oct 21, 2024 •

edited

Loading