Ejh 20160901 issue7 -- Backend Server basics

ejhumphrey commented 7 years ago

Will close #7, implements a bunch of good stuff

audio upload endpoint
audio download endpoint
polymorphic local variants of gcloud's classes
common interface for swapping gcloud / local backends
test coverage for library tools and main server

This change is

ejhumphrey commented 7 years ago

alrighty, now that everything rinses clear, someone might want to give this a looksee? maybe @stefan-balke?

stefan-balke commented 7 years ago

alrighty, now that everything rinses clear, someone might want to give this a looksee? maybe @stefan-balke?

Will take me some time, until Friday I should make it. Maybe next time some smaller PRs? :)

ejhumphrey commented 7 years ago

hehe

smaller PRs, agreed / I'm sorry 😄
no rush, we're ahead of schedule on this piece of the puzzle.
it would be great if anyone else who wants to be involved with the backend machinery could give this a look as well!

ejhumphrey commented 7 years ago

would like to bring this PR back soon so we can start carving out simpler next steps for improving upon what's there ... let's say a 1-2 day window for comments (?) and I'll sail this through

stefan-balke commented 7 years ago

backend_server/main.py, line 53 at r3 (raw file):

@app.route('/')
def hello():
    return 'oh hai'

Route can be deleted.

Comments from Reviewable

stefan-balke commented 7 years ago

backend_server/main.py, line 56 at r3 (raw file):


@app.route('/audio/upload', methods=['POST'])

add versioning.

@app.route('api/v1.0/audio/upload', methods=['POST'])

Won't cost much and this is not the way to do it (there exist a prefix command for this in flask), but before we edit it in the client code over and over again...

Comments from Reviewable

stefan-balke commented 7 years ago

backend_server/main.py, line 68 at r3 (raw file):

    """
    audio_data = request.files['audio']
    bytestring = audio_data.stream.read()

Check for file extensions or mime-types to make sure only audio files we support are uploaded

Comments from Reviewable

stefan-balke commented 7 years ago

backend_server/main.py, line 98 at r3 (raw file):


@app.route('/audio/<uri>', methods=['GET'])

api versioning

Comments from Reviewable

stefan-balke commented 7 years ago

backend_server/main.py, line 135 at r3 (raw file):


@app.route('/annotation/submit', methods=['POST'])

api versioning

Comments from Reviewable

stefan-balke commented 7 years ago

backend_server/main.py, line 163 at r3 (raw file):


@app.route('/annotation/taxonomy', methods=['GET'])

api versioning

Comments from Reviewable

stefan-balke commented 7 years ago

backend_server/pybackend/storage.py, line 1 at r3 (raw file):

from gcloud import storage

Maybe some general module comments here?

Comments from Reviewable

stefan-balke commented 7 years ago

backend_server/pybackend/storage.py, line 17 at r3 (raw file):


class LocalData(object):

this is a style question: Do you document class headers or only the procedures?

Comments from Reviewable

stefan-balke commented 7 years ago

As the project is at such an early stage, I think the most important point is to fix the API endpoints and keep a list of some todos to enhance this (API key, SSL, etc.).

ejhumphrey commented 7 years ago

backend_server/main.py, line 53 at r3 (raw file):

Previously, stefan-balke (Stefan Balke) wrote…

Route can be deleted.

Done.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/main.py, line 56 at r3 (raw file):

Previously, stefan-balke (Stefan Balke) wrote…

add versioning. ``` @app.route('api/v1.0/audio/upload', methods=['POST']) ``` Won't cost much and this is not the way to do it (there exist a prefix command for this in flask), but before we edit it in the client code over and over again...

Done.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/main.py, line 98 at r3 (raw file):

Previously, stefan-balke (Stefan Balke) wrote…

api versioning

Done.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/main.py, line 68 at r3 (raw file):

Previously, stefan-balke (Stefan Balke) wrote…

Check for file extensions or mime-types to make sure only audio files we support are uploaded

Done.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/main.py, line 135 at r3 (raw file):

Previously, stefan-balke (Stefan Balke) wrote…

api versioning

Done.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/main.py, line 163 at r3 (raw file):

Previously, stefan-balke (Stefan Balke) wrote…

api versioning

Done.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/pybackend/storage.py, line 17 at r3 (raw file):

Previously, stefan-balke (Stefan Balke) wrote…

this is a style question: Do you document class headers or only the procedures?

I have no strong feelings about this ... I usually try for at least the class constructor, and the class header when the interface (methods and attrs) stabilize. Have you any thoughts / preferences?

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/pybackend/storage.py, line 1 at r3 (raw file):

Previously, stefan-balke (Stefan Balke) wrote…

Maybe some general module comments here?

Done.

Comments from Reviewable

ejhumphrey commented 7 years ago

thanks!! :relaxed:

agreed! it's my hope that as the components become more established, it'll be easier to parallelize the development effort. do you think the TODOs should be inline comments, issues in github, or both?

Comments from Reviewable

stefan-balke commented 7 years ago

At the moment, we have github issues. I think general concepts/todos should be listed there. In my experience, TODOs in the source code are practically never touched again :)

Comments from Reviewable

stefan-balke commented 7 years ago

backend_server/pybackend/storage.py, line 17 at r3 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…

I have no strong feelings about this ... I usually try for at least the class constructor, and the class header when the interface (methods and attrs) stabilize. Have you any thoughts / preferences?

+1 for class constructor.

Comments from Reviewable

ejhumphrey commented 7 years ago

not unless a linter is configured to yell about them on a regular basis :o)

gh issues it is. will finish cleaning this up / making sure all tests pass and will lob back against this. thx again for the review!

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/pybackend/storage.py, line 17 at r3 (raw file):

Previously, stefan-balke (Stefan Balke) wrote…

+1 for class constructor.

Done.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/pybackend/database.py, line 56 at r1 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…

ah yes. The put/get interface for datastore requires some wrangling in terms of how Key objects are constructed. This interfaces serves to abstract some of that away in an application-specific manner. will update docstring accordingly.

Done.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/pybackend/database.py, line 69 at r1 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…

fix

punting for now, there's a larger gcloud integration issue here.

Comments from Reviewable

ejhumphrey commented 7 years ago

alrighty, I think this is in a good state for where we are. thoughts @stefan-balke ?

Comments from Reviewable

coveralls commented 7 years ago

Changes Unknown when pulling 88cdac43c1c41edf1705aed7fd5ec330dd0f769b on ejh_20160901_issue7 into on master.

karolpiczak commented 7 years ago

Finally had some time to clone it locally. Should be easier to participate now. Looks good to me, just dropping some minor comments.

Reviewed 9 of 18 files at r1, 1 of 1 files at r3, 8 of 8 files at r4. Review status: all files reviewed at latest revision, 4 unresolved discussions.

backend_server/main.py, line 77 at r4 (raw file):


    uri = str(pybackend.utils.uuid(bytestring))
    fext = os.path.splitext(audio_data.filename)[-1]

Nitpicking a bit, but it seems redundant with file_ext being already available.

backend_server/main.py, line 78 at r4 (raw file):

    uri = str(pybackend.utils.uuid(bytestring))
    fext = os.path.splitext(audio_data.filename)[-1]
    filepath = "{}{}".format(uri, fext)

filepath = "{}.{}".format(uri, file_ext)

backend_server/tests/test_main.py, line 28 at r4 (raw file):

def test_audio_upload_bad_request(app):
    r = app.get('/api/v0.1/audio/upload')
    assert r.status_code != 405

Seems confusing, a typo here?

backend_server/tests/test_main.py, line 37 at r4 (raw file):


def test_audio_get(app):

How about some simple content checks?

    # First we'll generate data...
    content = b'my new file contents'
    data = dict(audio=(BytesIO(content), 'blah.wav'))
    r = app.post('/api/v0.1/audio/upload', data=data)
    assert r.status_code == 200
    uri = json.loads(r.data.decode('utf-8'))['uri']

    # Now let's go looking for it
    r = app.get('/api/v0.1/audio/{}'.format(uri))
    assert r.status_code == 200
    assert r.data == content

Comments from Reviewable

stefan-balke commented 7 years ago

Comments from Reviewable

bmcfee commented 7 years ago

Review status: all files reviewed at latest revision, 7 unresolved discussions.

.travis.yml, line 27 at r4 (raw file):

  - pip install --upgrade protobuf
  - pip install coveralls
  - pip install -U -r backend_server/requirements/setup/requirements_dev.txt

Seems to me like these dependencies should be in a requires_extras field of setup.py, right?

backend_server/main.py, line 64 at r4 (raw file):

    """
    audio_data = request.files['audio']
    file_ext = os.path.splitext(audio_data.filename)[-1].strip('.')

stripping '.' should not be necessary here. Even so, you should use os.path.extsep instead of '.'.

backend_server/main.py, line 78 at r4 (raw file):

Previously, karoldvl (Karol J. Piczak) wrote…

`filepath = "{}.{}".format(uri, file_ext)`

os.path.extsep.join([uri, file_ext]) is the cross-platform-friendly way to do this

backend_server/pybackend/mime.py, line 11 at r4 (raw file):

import os

MIMETYPES = {

shouldn't this be handled by the mimetypes core lib? https://docs.python.org/2/library/mimetypes.html

Comments from Reviewable

ejhumphrey commented 7 years ago

.travis.yml, line 27 at r4 (raw file):

Previously, bmcfee (Brian McFee) wrote…

Seems to me like these dependencies should be in a `requires_extras` field of setup.py, right?

couple things:

there isn't a setup.py currently; not sure we need one (tools are only used by the app?)
opted for requirements files to handle deps
IIRC, some of this tomfoolery was to appease the travis gods

couple questions

is there a Right / Better Way to do this?
and if so, do you have any references to share for my education?

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/main.py, line 64 at r4 (raw file):

Previously, bmcfee (Brian McFee) wrote…

stripping `'.'` should not be necessary here. Even so, you should use `os.path.extsep` instead of `'.'`.

I think it is? os.path.splitext('file.wav') returns ('file', '.wav'). otherwise, the file extensions in the AUDIO_EXTENSIONS list need extseps prepended

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/main.py, line 77 at r4 (raw file):

Previously, karoldvl (Karol J. Piczak) wrote…

Nitpicking a bit, but it seems redundant with `file_ext` being already available.

ah, so it is now, good catch

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/main.py, line 78 at r4 (raw file):

Previously, bmcfee (Brian McFee) wrote…

`os.path.extsep.join([uri, file_ext])` is the cross-platform-friendly way to do this

Done.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/pybackend/mime.py, line 11 at r4 (raw file):

Previously, bmcfee (Brian McFee) wrote…

shouldn't this be handled by the `mimetypes` core lib? https://docs.python.org/2/library/mimetypes.html

wat the wat

Comments from Reviewable

bmcfee commented 7 years ago

Review status: all files reviewed at latest revision, 7 unresolved discussions.

.travis.yml, line 27 at r4 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…

couple things: - there isn't a `setup.py` currently; not sure we need one (tools are only used by the app?) - opted for requirements files to handle deps - IIRC, some of this tomfoolery was to appease the travis gods couple questions - is there a Right / Better Way to do this? - and if so, do you have any references to share for my education?

I generally prefer setup.py to requirements.txt for exactly this reason: you can separately define requirements for optional extensions, and then pip install -e .[tests,docs] (for example). librosa does this, see here.

backend_server/main.py, line 64 at r4 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…

I think it is? `os.path.splitext('file.wav')` returns `('file', '.wav')`. otherwise, the file extensions in the `AUDIO_EXTENSIONS` list need `extsep`s prepended

Derp, you're right. You could also just do os.path.splitext(fname)[1][1:], since multiple separators are joined to the prefix and not the extension. This would avoid explicitly naming the separator.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/pybackend/mime.py, line 11 at r4 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…

wat the wat

rolled the function into utils, rm'ed this

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/tests/test_main.py, line 28 at r4 (raw file):

Previously, karoldvl (Karol J. Piczak) wrote…

Seems confusing, a typo here?

mm, yes, agreed!

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/tests/test_main.py, line 37 at r4 (raw file):

Previously, karoldvl (Karol J. Piczak) wrote…

How about some simple content checks? ```python # First we'll generate data... content = b'my new file contents' data = dict(audio=(BytesIO(content), 'blah.wav')) r = app.post('/api/v0.1/audio/upload', data=data) assert r.status_code == 200 uri = json.loads(r.data.decode('utf-8'))['uri'] # Now let's go looking for it r = app.get('/api/v0.1/audio/{}'.format(uri)) assert r.status_code == 200 assert r.data == content ```

Done.

Comments from Reviewable

coveralls commented 7 years ago

Changes Unknown when pulling 9dd25a29bca6112511659d9e48f3df945eeb5fa5 on ejh_20160901_issue7 into on master.

karolpiczak commented 7 years ago

Reviewed 4 of 4 files at r5. Review status: all files reviewed at latest revision, 5 unresolved discussions.

backend_server/tests/test_main.py, line 28 at r4 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…

mm, yes, agreed!

Oh, I thought the intention was to expect a 405 error in here?

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/main.py, line 64 at r4 (raw file):

Previously, bmcfee (Brian McFee) wrote…

Derp, you're right. You could also just do `os.path.splitext(fname)[1][1:]`, since multiple separators are joined to the prefix and not the extension. This would avoid explicitly naming the separator.

I like it, fixing.

Comments from Reviewable

coveralls commented 7 years ago

Changes Unknown when pulling 80d7f6739268bea011d9f7bac846fa8adc1c9546 on ejh_20160901_issue7 into on master.

ejhumphrey commented 7 years ago

.travis.yml, line 27 at r4 (raw file):

Previously, bmcfee (Brian McFee) wrote…

I generally prefer setup.py to requirements.txt for exactly this reason: you can separately define requirements for optional extensions, and then `pip install -e .[tests,docs]` (for example). librosa does this, see [here](https://github.com/librosa/librosa/blob/master/setup.py).

thanks, done now.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/tests/test_main.py, line 28 at r4 (raw file):

Previously, karoldvl (Karol J. Piczak) wrote…

Oh, I thought the intention was to expect a 405 error in here?

ha, yes! that is what it should be. fixing

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/tests/test_main.py, line 28 at r4 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…

ha, yes! that is what it should be. fixing

Done.

Comments from Reviewable

ejhumphrey commented 7 years ago

backend_server/pybackend/mime.py, line 11 at r4 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…

rolled the function into utils, rm'ed this

Done.

Comments from Reviewable

cosmir / openmic-annotator

Ejh 20160901 issue7 -- Backend Server basics #25