Add GCF imagemagick samples #1684

ace-n · 2018-09-05T05:14:27Z

TODO - add Kokoro test config. (@andrewsg this is good to review though.)

EDIT: Kokoro configs need to be added for GCF separately. We should do this in a separate PR.

Change-Id: Ie8aca88b0dc80926438c9e87b2e71071f8f294d6

Change-Id: I75ec970fd22c870664e4133f51562793a464df1f

Change-Id: I255301ab7218b591c6c61f7da059a3d577bd3186

functions/imagemagick/main.py

andrewsg · 2018-09-11T20:59:26Z

functions/imagemagick/main.py

+def blur_offensive_images(data, context):
+    file = data
+
+    # Exit if this is a deletion or a deploy event.


Are these the best canonical ways of determine what kind of event it is? It seems a little haphazard to check for a name or state in this way as the only method.

Given that we can configure triggers to fire only on certain events (e.g. file writes), probably not.

We can delete this code from the Node sample as well - but we'll want to explicitly specify which event to use in the docs.

What do you want to do in this situation then, leave it or change it somehow? We should be careful to follow the most straightforward, best-documented practice we can in either this or the Node case.

Oh, I see it's been changed in the latest revision. Never mind. Thanks!

functions/imagemagick/main.py

andrewsg · 2018-09-11T23:17:05Z

functions/imagemagick/main.py

+    print('Image %s was downloaded to %s.' % (file_name, temp_local_filename))
+
+    # Blur the image using ImageMagick.
+    subprocess.check_call([


Is there any in-Python way to do this, for instance by using Pillow or an ImageMagick binding for Python, that doesn't involve a subprocess call?

Yes - Python has Wand.

For consistency, we should also do this in Node.js - which uses gm

Great, wand is a good solution here.

functions/imagemagick/main_test.py

Change-Id: I31808597e16b7b2201adbd8a51fe84241c90e649

Change-Id: I465cb4531a024265f66f857cbd793949b597d252

Change-Id: Ie69ce88686481640da59207682b078b800631620

andrewsg

lgtm if remaining discussions are closed to your satisfaction.

andrewsg · 2018-09-14T22:25:17Z

functions/imagemagick/main.py

+# Blurs the given file using ImageMagick.
+def __blur_image(blob):
+    file_name = blob.name
+    temp_local_filename = '/tmp/%s' % os.path.basename(file_name)


Rather than hardcoding /tmp, can we use tempfile.mkdtemp to get a random, guaranteed-temporary directory? (note, we should clean this up afterwards as well)

(One reason is to avoid filename conflicts or race conditions.)

Done (with mkstemp instead.)

andrewsg · 2018-09-14T22:27:05Z

functions/imagemagick/main.py

+def blur_offensive_images(data, context):
+    file = data
+
+    # Exit if this is a deletion or a deploy event.


What do you want to do in this situation then, leave it or change it somehow? We should be careful to follow the most straightforward, best-documented practice we can in either this or the Node case.

andrewsg · 2018-09-14T22:27:43Z

functions/imagemagick/main.py

+    print('Image %s was downloaded to %s.' % (file_name, temp_local_filename))
+
+    # Blur the image using ImageMagick.
+    subprocess.check_call([


Great, wand is a good solution here.

Change-Id: I62036d693fcf2451fbd1129d6799a736852ea455

ace-n · 2018-09-14T23:18:24Z

Best-practices question: this function triggers twice - once when the original file is uploaded, and a second time when the file is blurred. (The Node sample also does this.)

Should we address this in the sample and/or docs?

andrewsg · 2018-09-14T23:20:48Z

Could we design it not to do that somehow?

…

On Fri, Sep 14, 2018 at 4:18 PM Ace Nassri ***@***.***> wrote: Best-practices question: this function triggers *twice* - once when the original file is uploaded, and a second time when the file is blurred. (The Node sample also does this.) Should we address this in the sample and/or docs? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1684 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAp51wYdUo4YAlyGfsvFWwfCrA9f0d6eks5ubDlFgaJpZM4WaMgw> .

ace-n · 2018-09-14T23:23:06Z

Probably the best workaround is to upload to a different bucket altogether...which has its own costs.

Maybe we should just call it out in the docs? (I'll ping @labtopia about/continue this thread internally.)

andrewsg · 2018-09-14T23:50:47Z

That's also the workaround I was thinking of. An alternative would be to prefix the names of blur uploads with a constant like "blurred-" and have the function look for that prefix and abort, with a comment explaining the issue. I would prefer some workaround or another be implemented because it seems like not fixing this would open the function up to an infinite loop if the content identifier malfunctions upstream, and that customers modifying this function to meet their needs could easily also run into a similar problem. The workaround can be commented to alert customers to the potential issue as well.

…

On Fri, Sep 14, 2018, 4:23 PM Ace Nassri ***@***.***> wrote: Probably the best workaround is to upload to a different bucket altogether...which has its own costs. Maybe we should just call it out in the docs? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1684 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAp511OEeHGy-2N8U_MvVGOGPIzd3KHDks5ubDpkgaJpZM4WaMgw> .

ace-n · 2018-09-17T21:33:21Z

In the documented use case, it seems like it wouldn't be an issue. (Eventually, the blurring will obscure the image so much that the Vision API doesn't detect anything.)

I agree we should document how to handle user-provided cases though. Would including a comment on this in the sample be enough?

andrewsg · 2018-09-18T17:27:19Z

I would personally prefer if we actually architected the sample to make certain it only runs once. This will make it clearer to the user how to do it than any comment, and also I'm iffy about relying on the vision detection algorithm to work and prevent an infininte loop 100% of the time. Andrew

…

On Mon, Sep 17, 2018 at 2:33 PM Ace Nassri ***@***.***> wrote: In the *documented* use case, it seems like it wouldn't be an issue. (Eventually, the blurring will obscure the image so much that the Vision API doesn't detect anything.) I agree we should document how to handle *user-provided* cases though. Would commenting on this in the sample be enough? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1684 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAp5107_2ZGDM2MZXjOrA-5llZ2-rsY4ks5ucBUtgaJpZM4WaMgw> .

ace-n · 2018-09-25T18:35:33Z

~~(Marking as DO NOT MERGE until @andrewsg's feedback is addressed.)~~

This sample uses a blurred- prefix to detect previously-blurred images. @jmdobry is that OK with you, or should I suggest something else? (e.g. moving blurred images to a different GCS bucket?)

Change-Id: Iaddf913fd9be99e7daff6caf45aac2a03c1dceeb

michaelawyu

LGTM :)

Ace Nassri added 3 commits August 30, 2018 19:23

Initial commit of imagemagick sample

16a674b

Change-Id: Ie8aca88b0dc80926438c9e87b2e71071f8f294d6

Add unit tests + get them passing

bba4ff8

Change-Id: I75ec970fd22c870664e4133f51562793a464df1f

Get sample working on GCF

17d860f

Change-Id: I255301ab7218b591c6c61f7da059a3d577bd3186

ace-n added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 5, 2018

ace-n requested a review from andrewsg September 5, 2018 05:14

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 5, 2018

ace-n removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 7, 2018

andrewsg requested changes Sep 11, 2018

View reviewed changes

Ace Nassri added 3 commits September 12, 2018 10:20

Address comments, pt 1

fb919df

Change-Id: I31808597e16b7b2201adbd8a51fe84241c90e649

Address comments, pt 2

ae0ccb8

Change-Id: I465cb4531a024265f66f857cbd793949b597d252

Use UserDict instead of DictObject

e5dbac0

Change-Id: Ie69ce88686481640da59207682b078b800631620

andrewsg approved these changes Sep 14, 2018

View reviewed changes

Address comments

a2b458c

Change-Id: I62036d693fcf2451fbd1129d6799a736852ea455

ace-n mentioned this pull request Sep 14, 2018

Use library in GCF ImageMagick tutorial GoogleCloudPlatform/nodejs-docs-samples#741

Merged

ace-n added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 25, 2018

Use format strings + make function idempotent

ca26a77

Change-Id: Iaddf913fd9be99e7daff6caf45aac2a03c1dceeb

ace-n added needs review and removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Sep 26, 2018

ace-n requested review from michaelawyu and jmdobry September 26, 2018 02:01

michaelawyu approved these changes Sep 26, 2018

View reviewed changes

ace-n merged commit 06cc18c into master Sep 27, 2018

ace-n deleted the gcf-imagick branch September 27, 2018 23:31

ace-n removed needs review labels Oct 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GCF imagemagick samples #1684

Add GCF imagemagick samples #1684

ace-n commented Sep 5, 2018 •

edited

Loading

andrewsg Sep 11, 2018

ace-n Sep 12, 2018

andrewsg Sep 14, 2018

andrewsg Sep 14, 2018

andrewsg Sep 11, 2018

ace-n Sep 12, 2018 •

edited

Loading

andrewsg Sep 14, 2018

andrewsg left a comment

andrewsg Sep 14, 2018

andrewsg Sep 14, 2018

ace-n Sep 14, 2018

andrewsg Sep 14, 2018

andrewsg Sep 14, 2018

ace-n commented Sep 14, 2018

andrewsg commented Sep 14, 2018 via email

ace-n commented Sep 14, 2018 •

edited

Loading

andrewsg commented Sep 14, 2018 via email

ace-n commented Sep 17, 2018 •

edited

Loading

andrewsg commented Sep 18, 2018 via email

ace-n commented Sep 25, 2018 •

edited

Loading

michaelawyu left a comment

Add GCF imagemagick samples #1684

Add GCF imagemagick samples #1684

Conversation

ace-n commented Sep 5, 2018 • 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

ace-n Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsg 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

ace-n commented Sep 14, 2018

andrewsg commented Sep 14, 2018 via email

ace-n commented Sep 14, 2018 • edited Loading

andrewsg commented Sep 14, 2018 via email

ace-n commented Sep 17, 2018 • edited Loading

andrewsg commented Sep 18, 2018 via email

ace-n commented Sep 25, 2018 • edited Loading

michaelawyu left a comment

Choose a reason for hiding this comment

ace-n commented Sep 5, 2018 •

edited

Loading

ace-n Sep 12, 2018 •

edited

Loading

ace-n commented Sep 14, 2018 •

edited

Loading

ace-n commented Sep 17, 2018 •

edited

Loading

ace-n commented Sep 25, 2018 •

edited

Loading