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

Added mimetype to file data sent to model #39

Merged
merged 3 commits into from
Aug 16, 2018
Merged

Added mimetype to file data sent to model #39

merged 3 commits into from
Aug 16, 2018

Conversation

ajbozarth
Copy link
Member

This is a follow up to IBM/MAX-Image-Caption-Generator#9 which added file extension verification using mimetype

Closes #38

This is a follow up to IBM/MAX-Image-Caption-Generator#9 which added file extension verification using mimetype
@ajbozarth ajbozarth added the bug Something isn't working label Aug 16, 2018
@ajbozarth ajbozarth self-assigned this Aug 16, 2018
@ajbozarth ajbozarth requested a review from MLnick August 16, 2018 01:03
app.py Outdated
@@ -120,7 +121,8 @@ def valid_file_ext(filename):

# Runs ML on given image
def run_ml(img_path):
img_file = {'image': open(img_path, 'rb')}
mime_type = mimetypes.guess_type(img_path)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested what happens when no mime-type can be guessed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested with the three valid extensions, any other extension would be rejected before getting to this line. Given this library guesses mime type by the file extension (IIUC), that should be enough. I originally used magic to do this (more robust, not a guess), but it requires installs outside pip, which I didn't want to add.

app.py Outdated
@@ -120,7 +121,8 @@ def valid_file_ext(filename):

# Runs ML on given image
def run_ml(img_path):
img_file = {'image': open(img_path, 'rb')}
mime_type = mimetypes.guess_type(img_path)[0]
img_file = {'image': (img_path, open(img_path, 'rb'), mime_type)}
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken the file handle returned by open is never closed ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, odd that no one ever caught that before, not that it matter much given this file is opened in a thread. I'll close it.

@MLnick
Copy link

MLnick commented Aug 16, 2018

I see the Python 2.7 build on Travis failed to produce output. Do we know the issue?

@MLnick
Copy link

MLnick commented Aug 16, 2018

Changes LGTM

@stevemar
Copy link

@MLnick the travis job failed in a weird way, i'm restarting it:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

app.py Outdated
img_file = {'image': open(img_path, 'rb')}
r = requests.post(url=ml_endpoint, files=img_file)
mime_type = mimetypes.guess_type(img_path)[0]
img_file = open(img_path, 'rb')

Choose a reason for hiding this comment

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

If you use with it'll be closed at the end of the block. See the last paragraph of https://docs.python.org/2/tutorial/inputoutput.html#methods-of-file-objects

    with open(img_path, 'rb') as img_file:
        file_form = {'image': (img_path, img_file, mime_type)}

     r = requests.post(url=ml_endpoint, files=file_form)
     cap_json = r.json()

@ajbozarth
Copy link
Member Author

Updated to use with per @stevemart comment

@@ -120,8 +120,10 @@ def valid_file_ext(filename):

# Runs ML on given image
def run_ml(img_path):
img_file = {'image': open(img_path, 'rb')}
r = requests.post(url=ml_endpoint, files=img_file)
mime_type = mimetypes.guess_type(img_path)[0]

Choose a reason for hiding this comment

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

reading the docs, it's fine to assume [0] here. cool.

https://docs.python.org/2/library/mimetypes.html#mimetypes.guess_type

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I looked into that to make sure and the return is always a "pair"

Copy link

@stevemar stevemar left a comment

Choose a reason for hiding this comment

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

this LGTM

r = requests.post(url=ml_endpoint, files=img_file)
mime_type = mimetypes.guess_type(img_path)[0]
with open(img_path, 'rb') as img_file:
file_form = {'image': (img_path, img_file, mime_type)}

Choose a reason for hiding this comment

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

i'm not sure if requests will like mime_type set to None if it can't guess the mimetype correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

the current default is None which is what causes the error. And IIUC mimetypes.guess_type() uses the file extension in the filename to guess, and jpg, jpeg, and png all work and thats all we need.

Choose a reason for hiding this comment

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

you could handle the case where someone submits a file with no extension :) but we're bikeshedding now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

valid extensions are already checked elsewhere in both the python code that calls this function and on the html form client-side, so I think we're good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants