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.

Sign up for GitHub

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

Jump to bottom

Fix/75 no mkdir when s3 #76

Merged
merged 2 commits into from
Feb 27, 2024
Merged

Fix/75 no mkdir when s3 #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aed4d6a
fix: don't call mkdir when fs is s3, which calls CreateBucket that's …
lchen-2101 Feb 27, 2024
4b20393
fix: don't call mkdir when fs is s3, which calls CreateBucket that's …
lchen-2101 Feb 27, 2024
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
5 changes: 3 additions & 2 deletions src/services/submission_processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,16 @@
from fastapi import HTTPException
import logging
from fsspec import AbstractFileSystem, filesystem
from config import settings
from config import settings, FsProtocol

log = logging.getLogger(__name__)


async def upload_to_storage(lei: str, submission_id: str, content: bytes, extension: str = "csv"):
try:
fs: AbstractFileSystem = filesystem(settings.upload_fs_protocol.value)
fs.mkdirs(f"{settings.upload_fs_root}/{lei}", exist_ok=True)
if settings.upload_fs_protocol == FsProtocol.FILE:
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the LocalFileSystem impl takes a auto_mkdir boolean which would create the directory on open if it doesn't exist, that could be passed in kwargs to open I think. If so, this might be a good way to avoid this impl specific if (which loses the abstractness which is really nice to have!). The kwarg would get ignored by S3. I'm not positive, just reading the API, but might be worth a look?

If that doesn't work, this looks good. Just was hoping to keep it all abstract but alas!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

really good suggestion, alas it doesn't work... I get an unexpected keyword argument, so unfortunately the s3fs doesn't ignore it...

Copy link
Contributor

Choose a reason for hiding this comment

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

BOOO. Oh well

fs.mkdirs(f"{settings.upload_fs_root}/{lei}", exist_ok=True)
with fs.open(f"{settings.upload_fs_root}/{lei}/{submission_id}.{extension}", "wb") as f:
f.write(content)
except Exception as e:
Expand Down
18 changes: 16 additions & 2 deletions tests/services/test_submission_processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import pytest
from unittest.mock import Mock, ANY
from pytest_mock import MockerFixture
from config import FsProtocol, settings
from services.submission_processor import upload_to_storage


Expand All @@ -20,12 +21,25 @@ def mock_fs_func(mocker: MockerFixture, mock_fs: Mock) -> Mock:

async def test_upload(mocker: MockerFixture, mock_fs_func: Mock, mock_fs: Mock):
with mocker.mock_open(mock_fs.open):
await upload_to_storage("test", "test", b"test content")
await upload_to_storage("test", "test", b"test content local")
mock_fs_func.assert_called()
mock_fs.mkdirs.assert_called()
mock_fs.open.assert_called_with(ANY, "wb")
file_handle = mock_fs.open()
file_handle.write.assert_called_with(b"test content")
file_handle.write.assert_called_with(b"test content local")


async def test_upload_s3_no_mkdir(mocker: MockerFixture, mock_fs_func: Mock, mock_fs: Mock):
default_fs_proto = settings.upload_fs_protocol
settings.upload_fs_protocol = FsProtocol.S3
with mocker.mock_open(mock_fs.open):
await upload_to_storage("test", "test", b"test content s3")
mock_fs_func.assert_called()
mock_fs.mkdirs.assert_not_called()
mock_fs.open.assert_called_with(ANY, "wb")
file_handle = mock_fs.open()
file_handle.write.assert_called_with(b"test content s3")
settings.upload_fs_protocol = default_fs_proto


async def test_upload_failure(mocker: MockerFixture, mock_fs_func: Mock, mock_fs: Mock):
Expand Down
Loading