cosmir / openmic-annotator

Annotation framework for annotating data for OpenMIC
MIT License
56 stars 1 forks source link

Ejh 20161202 annotator integration #36

Closed ejhumphrey closed 7 years ago

ejhumphrey commented 7 years ago

Note: The audio-annotator submodule was updated in the course of these changes.


This change is Reviewable

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 89.51% when pulling 0804e6d60eaec580434a4ed0dec67d3f307c9078 on ejh_20161202_annotator_integration into 38d7a6086cd993096e2ffcc51916f61a0a53765f on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 89.51% when pulling 0804e6d60eaec580434a4ed0dec67d3f307c9078 on ejh_20161202_annotator_integration into 38d7a6086cd993096e2ffcc51916f61a0a53765f on master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 89.51% when pulling 0804e6d60eaec580434a4ed0dec67d3f307c9078 on ejh_20161202_annotator_integration into 38d7a6086cd993096e2ffcc51916f61a0a53765f on master.

bmcfee commented 7 years ago

Reviewed 13 of 16 files at r1. Review status: 13 of 16 files reviewed at latest revision, 1 unresolved discussion.


README.md, line 60 at r1 (raw file):

This will start the backend server (CMS), upload a few audio files, and begin serving the audio annotation tool locally. By default this will appear at [http://localhost:8000/docs/annotator.html](http://localhost:8000/docs/annotator.html).

**Note**: For some reason, loading the audio seems to get "stuck" on occasion. To unblock it, manually proceed to an audio file's URL once the server is running, e.g. [http://localhost:8080/api/v0.1/audio/740c835f-a23d-41ef-b84c-0cd1de4edfa5](http://localhost:8080/api/v0.1/audio/740c835f-a23d-41ef-b84c-0cd1de4edfa5).

Is hard-coding this really useful? I imagine the hash could change, and break this link.


Comments from Reviewable

bmcfee commented 7 years ago

Review status: 13 of 16 files reviewed at latest revision, 2 unresolved discussions.


README.md, line 66 at r1 (raw file):

### Content Management System

See the [ReadMe](https://github.com/cosmir/open-mic/blob/master/backend_server/README.md) for details on running the backend web server.

Can this link be relative?


Comments from Reviewable

bmcfee commented 7 years ago

Review status: 13 of 16 files reviewed at latest revision, 3 unresolved discussions.


run_demo.sh, line 6 at r1 (raw file):

python backend_server/main.py --port 8080 --local --debug &
CMS_PID=$!
sleep 4s

This is okay for now, but this should really be a daemon that stores its PID to a user-specified pid file, which can be read once the initial process terminates.


Comments from Reviewable

ejhumphrey commented 7 years ago

README.md, line 60 at r1 (raw file):

Previously, bmcfee (Brian McFee) wrote…
Is hard-coding this really useful? I imagine the hash could change, and break this link.

UUIDs are deterministic based on the bytestring of the file; this points to a checked in file, and shouldn't change. That said, it's worth mentioning that in the README to prevent future confusion.


Comments from Reviewable

ejhumphrey commented 7 years ago

README.md, line 66 at r1 (raw file):

Previously, bmcfee (Brian McFee) wrote…
Can this link be relative?

¯_(ツ)_/¯ I'll look into it


Comments from Reviewable

ejhumphrey commented 7 years ago

run_demo.sh, line 6 at r1 (raw file):

Previously, bmcfee (Brian McFee) wrote…
This is okay for now, but this should really be a daemon that stores its PID to a user-specified pid file, which can be read once the initial process terminates.

I spent 10-15 minutes rooting around the interstacks, and didn't find an example I like -- do you have one in mind you could point to?


Comments from Reviewable

bmcfee commented 7 years ago

Review status: 13 of 16 files reviewed at latest revision, 1 unresolved discussion.


run_demo.sh, line 6 at r1 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…
I spent 10-15 minutes rooting around the interstacks, and didn't find an example I like -- do you have one in mind you could point to?

How about start-stop-daemon?


Comments from Reviewable

ejhumphrey commented 7 years ago

run_demo.sh, line 6 at r1 (raw file):

Previously, bmcfee (Brian McFee) wrote…
How about start-stop-daemon?

looks like that's linux only? suppose that would work for travis, but us OS X folks couldn't run it locally ... this is at least unix compliant?


Comments from Reviewable

ejhumphrey commented 7 years ago

Reviewed 2 of 16 files at r1. Review status: 15 of 16 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

bmcfee commented 7 years ago

Review status: 15 of 16 files reviewed at latest revision, 1 unresolved discussion.


run_demo.sh, line 6 at r1 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…
looks like that's linux only? suppose that would work for travis, but us OS X folks couldn't run it locally ... this is at least unix compliant?

Ugh, OSX. See thread: https://apple.stackexchange.com/questions/99888/is-there-any-port-of-start-stop-daemon-for-os-x

I guess that works via launchctl or launchd or something, but I have no way to verify. Sorry!


Comments from Reviewable

ejhumphrey commented 7 years ago

run_demo.sh, line 6 at r1 (raw file):

Previously, bmcfee (Brian McFee) wrote…
Ugh, OSX. See thread: https://apple.stackexchange.com/questions/99888/is-there-any-port-of-start-stop-daemon-for-os-x I guess that works via `launchctl` or `launchd` or something, but I have no way to verify. Sorry!

Doesn't seem to be the answer -- might need to use a different process shell?


Comments from Reviewable

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 89.51% when pulling 9ccd709279e45131fa9862f86e7bbd80b0a5eecf on ejh_20161202_annotator_integration into 38d7a6086cd993096e2ffcc51916f61a0a53765f on master.

ejhumphrey commented 7 years ago

@bmcfee can I get a fresh look on this? I swapped out the bash shell script demo in favor of a python version, which is probably (hopefully) preferable to the previous solution.


Comments from Reviewable

ejhumphrey commented 7 years ago

run_demo.sh, line 6 at r1 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…
Doesn't seem to be the answer -- might need to use a different process shell?

I've rm'ed this file in favor of demo.py


Comments from Reviewable

ejhumphrey commented 7 years ago

Reviewed 3 of 3 files at r3. Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 89.51% when pulling 9f0d61891efb195b3e5d31c324429482413e1643 on ejh_20161202_annotator_integration into 38d7a6086cd993096e2ffcc51916f61a0a53765f on master.

bmcfee commented 7 years ago

Reviewed 1 of 16 files at r1. Review status: all files reviewed at latest revision, 3 unresolved discussions.


demo.py, line 39 at r3 (raw file):

    try:
        input("Press return to exit.")
    except:

don't do a bare catch here; use the correct exception class


demo.py, line 42 at r3 (raw file):

        pass
    os.killpg(os.getpgid(server.pid), signal.SIGTERM)
    os.killpg(os.getpgid(webapp.pid), signal.SIGTERM)

put subprocess kills in a finally block


Comments from Reviewable

bmcfee commented 7 years ago

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


demo.py, line 1 at r3 (raw file):

#!/usr/bin/python

use #!/usr/bin/env python instead of #!/usr/bin/python to ensure proper environment capsulization


Comments from Reviewable

bmcfee commented 7 years ago

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


demo.py, line 25 at r3 (raw file):

                               '8080', '--local', '--debug'],
                              stdout=subprocess.PIPE, preexec_fn=os.setsid)
    time.sleep(4)

this still seems wrong, but we can revisit later.


Comments from Reviewable

ejhumphrey commented 7 years ago

demo.py, line 1 at r3 (raw file):

Previously, bmcfee (Brian McFee) wrote…
use #!/usr/bin/env python instead of #!/usr/bin/python to ensure proper environment capsulization

Done.


Comments from Reviewable

ejhumphrey commented 7 years ago

demo.py, line 42 at r3 (raw file):

Previously, bmcfee (Brian McFee) wrote…
put subprocess kills in a finally block

Done.


Comments from Reviewable

ejhumphrey commented 7 years ago

demo.py, line 25 at r3 (raw file):

Previously, bmcfee (Brian McFee) wrote…
this still seems wrong, but we can revisit later.

I did something smarter here, you might want to give this a look.


Comments from Reviewable

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.1%) to 89.51% when pulling dd38b99db6dfbca73805f068717b73d71d657c9b on ejh_20161202_annotator_integration into 38d7a6086cd993096e2ffcc51916f61a0a53765f on master.

ejhumphrey commented 7 years ago

demo.py, line 42 at r3 (raw file):

Previously, ejhumphrey (Eric J. Humphrey) wrote…
Done.

actually, didn't need that second try / except? I was getting some funny business from input(), but it seems to have resolved itself.


Comments from Reviewable

ejhumphrey commented 7 years ago

demo.py, line 39 at r3 (raw file):

Previously, bmcfee (Brian McFee) wrote…
don't do a bare catch here; use the correct exception class

unnec.


Comments from Reviewable

ejhumphrey commented 7 years ago

Reviewed 1 of 2 files at r4. Review status: 15 of 16 files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

ejhumphrey commented 7 years ago

tiny ping @bmcfee

bmcfee commented 7 years ago

Reviewed 1 of 3 files at r3, 2 of 2 files at r4. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

ejhumphrey commented 7 years ago

thanks, merging