Rewrite of storage backend using fs #145

rhodes73 · 2020-05-13T17:59:06Z

Thanks for creating imagetagger. It's a great tool. I use it together with Google Cloud Storage and I therefore did a rather large refactoring, abstracting the file storage layer of imagetagger.

Now many other filesystems like S3 or GCS are supported through PyFilesystem2 (see here for a list of supported file systems.)

There were no tests for the file storage layer and I did not write any new ones with this refactoring. I did some manual testing and am successfully using this filesystem abstraction in my Google Cloud deployment. Feel free to write some tests yourself. I might also add some later on.

I thought I contribute this potentially useful change back in case it is useful for other people. However, feel free to adapt or completely reject this pull request.

rhodes73 · 2020-05-13T18:07:44Z

There are still 3 open TODOs related to setting group permissions, where some input from @Akasch would be appreciated.

rhodes73 · 2020-05-13T18:50:03Z

imagetagger/imagetagger/images/views.py

+                                        image_file.seek(0)
+                                        imageset_dir.upload(img_fname, image_file)
+                                        # TODO rfrigg: Check if necessary
+                                        # shutil.chown(file_new_path, group=settings.UPLOAD_FS_GROUP)


@Akasch Can you explain why this is needed? I guess it might be needed for an OS filesystem, but not for a cloud filesystem like GCS. I can look into adding some code that adds this line back in case an OS filesystem is used.

@ftsell Shall we remove these TODOs since you tested everything and it works as expected?

Yes although I'm not 100% sure either, I think it is safe to do so.

rhodes73 · 2020-07-09T12:17:50Z

I'm closing this PR since there seems to be no interest in contributions.

ftsell · 2020-07-09T15:05:15Z

It's not that we are not interested but rather that there is currently not much activity because of COVID. I agree that we should have at least responded though. Be assured that it was not meant as a personal offense.

I'm reopening this because we still are interested in integrating this feature.

rhodes73 · 2020-07-09T19:39:52Z

@ftsell No offense taken :) Thanks for re-opening in this case.

ftsell

In general these are really nice improvements and I appreciate the effort you have put in; thank you very much!

I have tested the proposed changes by running manage.py directly and inside a kubernetes deployment but only with a local file-system. I do not expect cloud based storage to behave any different since that is the whole point of using fs.
In both scenarios I was able to verify that uploading, downloading, viewing and editing of tools and images work as expected. The zip-daemon threw a minor error which was easily correctable.

I have some minor suggestions which mostly have to do with where settings are stored.

I have also attached patch files which merge master back into your branch and fix the problems/suggestions I have found.

imagetagger/imagetagger/images/management/commands/runzipdaemon.py

imagetagger/imagetagger/settings_base.py

Now many other filesystems like S3 or GCS are supported through the fs filesystem abstraction layer.

previously, the zip-daemon would sometimes run into an error when the tmp-directory for a specific imageset did not already exist. A fix has been included to create the directory in such a case.

rhodes73 · 2020-10-02T12:49:31Z

@ftsell I rebased my branch on the latest master and applied your patch 008 for the zip daemon. Patches 001 until 006 are not different from the commits on master and were automatically introduced because of the rebase. 007 is unrelated to this PR, so I didn't apply it. 009 is about moving things from base_settings.py to settings.py.example, but this patch failed when I tried to apply it. Maybe easier if you just add separate PRs for patches 007 and 009.

ftsell · 2020-10-03T08:56:37Z

@roman-captural

007 is unrelated to this PR

Not exactly because the additional dependency is only required with the addition of fs. And because you are introducing the dependency to fs in your PR I would very much like to make sure that the application continues to work even in the docker deployment.

Opening another PR for improving the settings structure is something I can agree with so you can leave out patch 009 if you want to.

the dependency was only added to the Dockerfile and not requirements.txt since it is present in non-minimized python environments.

rhodes73 · 2020-10-03T10:08:51Z

Not exactly because the additional dependency is only required with the addition of fs. And because you are introducing the dependency to fs in your PR I would very much like to make sure that the application continues to work even in the docker deployment.

I see, sorry about this. It wasn't clear from the commit message. I added another commit with patch 007

ftsell · 2020-10-03T10:10:57Z

@roman-captural

It wasn't clear from the commit message.

true

ftsell

alright; looks fine to me 👍

I spoke with @timonegk and he wanted to do a review as well so that's why this is only a comment and not approval.

timonegk · 2020-10-04T15:19:20Z

Rebasing 1a0264f introduced a small problem in settings.py.example: the paths given for DATA_PATH, TOOLS_PATH and IMAGE_PATH appear to be absolute paths but are instead interpreted relative to FS_URL in the code.

ftsell · 2020-10-08T08:52:13Z

Rebasing 1a0264f introduced a small problem in settings.py.example: the paths given for DATA_PATH, TOOLS_PATH and IMAGE_PATH appear to be absolute paths but are instead interpreted relative to FS_URL in the code.

@timonegk is right but something simple like this should be enough though.

rhodes73 · 2020-10-08T12:56:28Z

@timonegk Thanks for your review and for testing this again after the rebase. I'm currently a bit short on time to look at it in detail. I applied @ftsell's patch (thanks!). I hope everything is functional now, so we can get this PR merged.

timonegk · 2020-10-08T13:42:59Z

Looks great now, thank you for the feature and sorry again for the long delay!

rhodes73 commented May 13, 2020

View reviewed changes

rhodes73 closed this Jul 9, 2020

ftsell reopened this Jul 9, 2020

ftsell self-requested a review October 2, 2020 09:39

ftsell requested changes Oct 2, 2020

View reviewed changes

imagetagger/imagetagger/images/management/commands/runzipdaemon.py Show resolved Hide resolved

imagetagger/imagetagger/settings_base.py Show resolved Hide resolved

Roman Frigg and others added 2 commits October 2, 2020 14:41

Rewrite of storage backend using fs

dd76529

Now many other filesystems like S3 or GCS are supported through the fs filesystem abstraction layer.

fix zip-daemon tmp-dir-not-exists error

757cb2f

previously, the zip-daemon would sometimes run into an error when the tmp-directory for a specific imageset did not already exist. A fix has been included to create the directory in such a case.

rhodes73 force-pushed the abstract-filesystems branch from 47898e2 to 757cb2f Compare October 2, 2020 12:43

add additional dependecy in Dockerfile

ca683d8

the dependency was only added to the Dockerfile and not requirements.txt since it is present in non-minimized python environments.

ftsell self-requested a review October 3, 2020 10:12

ftsell reviewed Oct 3, 2020

View reviewed changes

make paths non-absolute to work good with fs

f7f5cfa

timonegk approved these changes Oct 8, 2020

View reviewed changes

ftsell approved these changes Oct 8, 2020

View reviewed changes

ftsell merged commit 6a4e5e0 into bit-bots:master Oct 8, 2020

rhodes73 deleted the abstract-filesystems branch October 8, 2020 14:23

timonegk mentioned this pull request Nov 8, 2020

Fix error when tool filename is None #156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite of storage backend using fs #145

Rewrite of storage backend using fs #145

rhodes73 commented May 13, 2020

rhodes73 commented May 13, 2020

rhodes73 May 13, 2020

rhodes73 Oct 3, 2020

ftsell Oct 3, 2020

rhodes73 commented Jul 9, 2020

ftsell commented Jul 9, 2020

rhodes73 commented Jul 9, 2020

ftsell left a comment

rhodes73 commented Oct 2, 2020

ftsell commented Oct 3, 2020

rhodes73 commented Oct 3, 2020 •

edited

Loading

ftsell commented Oct 3, 2020

ftsell left a comment

timonegk commented Oct 4, 2020

ftsell commented Oct 8, 2020

rhodes73 commented Oct 8, 2020

timonegk commented Oct 8, 2020

Rewrite of storage backend using fs #145

Rewrite of storage backend using fs #145

Conversation

rhodes73 commented May 13, 2020

rhodes73 commented May 13, 2020

rhodes73 May 13, 2020

Choose a reason for hiding this comment

rhodes73 Oct 3, 2020

Choose a reason for hiding this comment

ftsell Oct 3, 2020

Choose a reason for hiding this comment

rhodes73 commented Jul 9, 2020

ftsell commented Jul 9, 2020

rhodes73 commented Jul 9, 2020

ftsell left a comment

Choose a reason for hiding this comment

rhodes73 commented Oct 2, 2020

ftsell commented Oct 3, 2020

rhodes73 commented Oct 3, 2020 • edited Loading

ftsell commented Oct 3, 2020

ftsell left a comment

Choose a reason for hiding this comment

timonegk commented Oct 4, 2020

ftsell commented Oct 8, 2020

rhodes73 commented Oct 8, 2020

timonegk commented Oct 8, 2020

rhodes73 commented Oct 3, 2020 •

edited

Loading