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.

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

Add GCF imagemagick samples #1684

Merged
merged 8 commits into from
Sep 27, 2018
Merged

Add GCF imagemagick samples #1684

merged 8 commits into from
Sep 27, 2018

Conversation

ace-n
Copy link

@ace-n ace-n commented Sep 5, 2018

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.

Ace Nassri added 3 commits August 30, 2018 19:23
Change-Id: Ie8aca88b0dc80926438c9e87b2e71071f8f294d6
Change-Id: I75ec970fd22c870664e4133f51562793a464df1f
Change-Id: I255301ab7218b591c6c61f7da059a3d577bd3186
@ace-n 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 ace-n requested a review from andrewsg September 5, 2018 05:14
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 5, 2018
@ace-n 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
functions/imagemagick/main.py Outdated Show resolved Hide resolved
def blur_offensive_images(data, context):
file = data

# Exit if this is a deletion or a deploy event.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

functions/imagemagick/main.py Outdated Show resolved Hide resolved
functions/imagemagick/main.py Outdated Show resolved Hide resolved
print('Image %s was downloaded to %s.' % (file_name, temp_local_filename))

# Blur the image using ImageMagick.
subprocess.check_call([
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@ace-n ace-n Sep 12, 2018

Choose a reason for hiding this comment

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

Yes - Python has Wand.

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

Copy link
Member

Choose a reason for hiding this comment

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

Great, wand is a good solution here.

functions/imagemagick/main_test.py Outdated Show resolved Hide resolved
Ace Nassri added 3 commits September 12, 2018 10:20
Change-Id: I31808597e16b7b2201adbd8a51fe84241c90e649
Change-Id: I465cb4531a024265f66f857cbd793949b597d252
Change-Id: Ie69ce88686481640da59207682b078b800631620
Copy link
Member

@andrewsg andrewsg left a comment

Choose a reason for hiding this comment

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

lgtm if remaining discussions are closed to your satisfaction.

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

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Done (with mkstemp instead.)

def blur_offensive_images(data, context):
file = data

# Exit if this is a deletion or a deploy event.
Copy link
Member

Choose a reason for hiding this comment

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

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.

print('Image %s was downloaded to %s.' % (file_name, temp_local_filename))

# Blur the image using ImageMagick.
subprocess.check_call([
Copy link
Member

Choose a reason for hiding this comment

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

Great, wand is a good solution here.

Change-Id: I62036d693fcf2451fbd1129d6799a736852ea455
@ace-n
Copy link
Author

ace-n commented Sep 14, 2018

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
Copy link
Member

andrewsg commented Sep 14, 2018 via email

@ace-n
Copy link
Author

ace-n commented Sep 14, 2018

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
Copy link
Member

andrewsg commented Sep 14, 2018 via email

@ace-n
Copy link
Author

ace-n commented Sep 17, 2018

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
Copy link
Member

andrewsg commented Sep 18, 2018 via email

@ace-n 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
@ace-n
Copy link
Author

ace-n commented Sep 25, 2018

(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
@ace-n 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
Copy link
Contributor

@michaelawyu michaelawyu left a comment

Choose a reason for hiding this comment

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

LGTM :)

@ace-n ace-n merged commit 06cc18c into master Sep 27, 2018
@ace-n ace-n deleted the gcf-imagick branch September 27, 2018 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants