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

Mb web interface #2327

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@tigranl
Copy link
Contributor

commented Dec 17, 2016

Added http server.

@Freso
Copy link
Member

left a comment

I like that the server only runs while importing, however, how will it behave when port 8000 is used by another process (e.g., if Picard is also running)?

Also, new code should generally be accompanied by tests too.

@@ -545,7 +545,11 @@ def choose_candidate(candidates, singleton, rec, cur_artist=None,
assert not singleton
return importer.action.TRACKS
elif sel == u'e':
return importer.action.MANUAL
ans = ui.input_("Use MB web interface?")

This comment has been minimized.

Copy link
@Freso

Freso Dec 17, 2016

Member

"MB" should probably be spelled out.

@@ -0,0 +1,22 @@
import socket

This comment has been minimized.

Copy link
@Freso

Freso Dec 17, 2016

Member

You should add a copyright blurb, encoding info, and docstring to the file as well. (See other files.)

import socket


class Server:

This comment has been minimized.

Copy link
@Freso

Freso Dec 17, 2016

Member

Remember to include a docstring.

self.port = port
self.start = None

def start_server(self):

This comment has been minimized.

Copy link
@Freso

Freso Dec 17, 2016

Member

Docstring…

self.start.bind((self.host, self.port))
self.start.listen(1)

def listen(self):

This comment has been minimized.

Copy link
@Freso

Freso Dec 17, 2016

Member

… and docstring here too.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 17, 2016

Hey! Thanks for forging right ahead on this. Can we chat a little about the design before you finish up?

In particular, I think this should be a plugin. Plugins can add options to the importer action prompt, so we can directly insert an option to launch the browser interface from there. This will avoid bothering people who don't need that option—for example, when running beets in an ssh instance on a remote server.

Do you have ideas about how you'll finish up the implementation? For example:

  • You'll want to let the user open their browser to the appropriate page. This can be done just by printing a URL or by hooking into the OS to actually open it. The latter would be more convenient.
  • To finish the server component, you can either do things manually or use a dependency such as Flask or the underlying Werkzeug library. The latter would probably make things simpler, but I wouldn't rule out the former if you want to avoid the dependency. Keeping things in a plugin will also avoid adding a dependency to beets core.
@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2016

Your last point is exactly what I thought. It's just a tiny http server, so there is no need to use Flask.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 17, 2016

Oh yeah! I also meant to add that the edit plugin uses a custom importer prompt option—you might take a look there for inspiration.

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2016

By the way, I was wondering why do you use Enum in the project? As far as I'm concerned, enum was added in the 3.4 version and it isn't supported in the 2.* version of python.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 17, 2016

On 2.7, we use enum34, the backport available on PyPI.

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2016

Hi, @sampsyo. How do you set up plugins? I have created beetsplug directory with __init__.py and web_tagger.py. And I have also appended directory of plugin to PYTHONPATH:
export PYTHONPATH=$PYTHONPATH:/home/dragonfly/beetsplug

But I am getting ImportError:
** error loading plugin web_tagger: Traceback (most recent call last): File "/usr/lib/python3.5/site-packages/beets-1.4.2-py3.5.egg/beets/plugins.py", line 254, in load_plugins namespace = __import__(modname, None, None) ImportError: No module named 'beetsplug.web_tagger'

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 19, 2016

Hi! The easiest way is probably just to use your existing checkout of the beets source (which I'm presuming you've "installed" with pip install -e .) and just create a new Python file in its beetsplug directory. This should get picked up automatically. That is, you can just create mbweb.py or whatever alongside edit.py and all the rest.

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2016

That's what I did. I have a question about stages: can I just use threading instead of it? Or is it necessary to use due to beets architecture?

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 19, 2016

It's probably a better idea to use the special beets hooks for anything that can use them. But what were you imagining to do by using threading directly? I'd be happy to discuss in more detail if you have a specific proposal.

For this particular feature, however, I believe everything will need to take place on the "UI thread." That is, it should happen synchronously while the user is considering a single album—I can't see any obvious way to add parallelism here.

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2016

I thought about running server in the separate thread, so the user can always choose another release. If the server is running in the UI thread you must rerun server every time you want to pick different track.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 19, 2016

OK, I see. So the server would always be running in the background, but it would only be "meaningful" if there's some release currently being considered in the UI stage. Is that right?

That could make sense, although it could be a bit more complex than starting the server only when a choice is needed. This could be accomplished by starting the server in a background thread when the importer starts and shutting it down when it finishes.

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2016

Yep, that's right. Do you think it would be an appropriate solution?

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

Sure! I'd advocate for whichever solution you find leads to the simplest code—the amount of functionality doesn't seem that different, overall.

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2016

Some initial code: https://github.com/tigranl/beets-web-tagger
Sorry, didn't have time to code yesterday.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

Oh, very nifty! Thanks for sending this along.

It looks like you have a little more coding to do (or, if I'm wrong, please feel free to summarize where you currently are). It also looks like the standard-library HTTPServer might be useful here?

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2016

There still is some coding to do. I need to figure out how to pass user's 'Look up' choice to importer.action.MANUAL. Yeah, I thought about using HTTPServer, but I want to use sockets, because I have never actually tried it.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 21, 2016

Cool!

For what it's worth, the function should probably return an autotag.AlbumMatch object, like this line that returns such an object during the normal tagging flow.

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2016

Hi, @sampsyo. As you know, threads in python are implemented using GIL, so there is no way to run two threads simultaneously. But in my case I need to run server thread and beet main thread. So, I guess, the best idea would be to run server every time user wants to choose a track, or to use multiprocessing if user set up this option in the config file.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 24, 2016

Hi! The GIL actually shouldn't be a problem here—it only becomes an issue if you want code to run faster by taking advantage of multiple cores. It won't stop you from running a web server on one thread and simultaneously doing other beets stuff in the other thread. The web server thread can be waiting for connections, for example, while the other thread makes progress.

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2016

Should I create two separate threads with MBWeb class and ThreadedServer and then join() them?

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 25, 2016

Well, you don't actually need two threads—the main code is already running its own thread (implicitly). You just need to fork, and then eventually join, the second thread. Here's a tiny example, at the risk of being redundant:

import threading
import time


class MyThread(threading.Thread):
    def run(self):
        print('start thread')
        time.sleep(2)
        print('end thread')


def main():
    t = MyThread()
    print('before start')
    t.start()
    t.join()
    print('after end')


if __name__ == '__main__':
    main()
@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2016

Hi! Merry Christmas and a Happy New Year! 🎄
I am having trouble getting socket's listen run second time, can you give some advice on this?https://gist.github.com/tigranl/ccc7e26d23efa001ac25af125d099ee4

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 26, 2016

Happy holidays to you too!

Sure; I'd be happy to help! What's going wrong with that code? Is there anything I should try running locally, or maybe an error message I should look for?

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 26, 2016

Try to run it locally.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 26, 2016

OK—specifically, I should copy that gist into my beetsplug folder, enable the plugin, import something, and type l or whatever?

Any other context about what's going wrong?

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 26, 2016

Cool! I'm getting this set up.

Along the way, I noticed this line:

PORT = config['web_tagger']['port'].as_number() or 8000

which, for consistency, should go in the plugin's __init__ instead of in the globals for this module. It will also need a default value set up there.

In addition, the urllib.parse import will need to be made Py2-compatible.

It occurs to me (too late, of course) that I should have suggested opening a PR with this instead of using a gist, which makes code reviews much easier. Next time!

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 26, 2016

Here's what's going on: it looks like the plugin currently listens for a connection, receives the HTTP request, and parses it (by hand). It doesn't send a response.

When the browser tries to connect to the tagger to issue the request from the "tagger" link, it sends a request and waits for the response (just as with other HTTP requests). But the connection unexpectedly drops, so the browser thinks something has gone wrong. I believe it then retries the request, which means multiple requests are sent. The second time the plugin accepts an incoming connection, it gets the retried request.

I think the way to solve this is to send an appropriate HTTP response. For that, using the full HTTPServer deal might be the right thing to do here instead of hand-crafting an HTTP server.

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2016

I should have pushed PR earlier, my bad.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

No worries! Did my attempt at an explanation above make sense to you?

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2016

Yes, after adding socket send function it now works as expected. But I'm not sure about _get_plugin_choices argument, because it throws TypeError: _get_plugin_choices() missing 1 required positional argument: 'task'.
Also, i can't create PR with beetsplug directory, so I just added main script without __init__.py.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

Great! A single-file plugin is just fine in this case.

Python is telling you this won't work:

TerminalImportSession._get_plugin_choices(task)

because that's an instance method, not a static method, so it can't be called on the class itself.

To make this work cleanly, we may want to extend the interface in the importer itself. It would be nice if the plugin didn't have to explicitly invoke the importer choice logic—if it could just return the ID in question and let the importer take care of the rest. That would avoid the whole problem of invoking stuff in TerminalImportSession altogether.

I'll look into this right now.

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2016

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

OK, I've pushed a bit of refactoring that should make this easy! Now, the built-in option for searching by ID works using exactly the same functionality as a plugin. See this function here: https://github.com/beetbox/beets/blob/master/beets/ui/commands.py#L692-L707

That's a callback from a PromptChoice. As you can see, it just returns a Proposal value (also new, but it's just the return value from tag_album or tag_item) to expose the new possible matches to the user. Your plugin can use the same strategy: just call tag_abum (or tag_item in singleton mode) with the ID retrieved by the Web interface and return the Proposal. The normal workflow should take care of everything from there.

@tigranl tigranl force-pushed the tigranl:MB_web_interface branch from f81066c to f3b30a9 Dec 29, 2016

@sampsyo
Copy link
Member

left a comment

OK, looking good so far! This has made a lot of progress.

Before this can be merged, it will need more comments in the code and a documentation page. If you like, though, we can call this GCI task done.

from beets import ui


PORT = 8000 # Temporary for PR

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 29, 2016

Member

This should probably be configurable.

else:
break
parsed = urlparse.urlparse(url)
return str(urlparse.parse_qs(parsed.query)['id'][0])

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 29, 2016

Member

Comments for this funky parsing code would be really helpful.

This comment has been minimized.

Copy link
@tigranl

tigranl Dec 29, 2016

Author Contributor

I don't like this function very much. Any ideas how to rewrite? I am thinking of using regexp.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 29, 2016

Member

Sure, a regular expression would make sense. Can you give me an idea of what the code's supposed to do (it's a little hard to read without context)?

This comment has been minimized.

Copy link
@tigranl

tigranl Dec 29, 2016

Author Contributor

It retrieves Musicbrainz id from data received from the socket. Function makes the list of received bytes and picks up the first value (which is a GET method), then using for loop it catches URI, and finally parses id parameter with urlparse.

I hope my explanation makes things clearer.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 29, 2016

Member

Aha, got it! It seems like the right thing to do is to use a real HTTP parser then. Here's one way to do it, if you want to avoid using a full HTTPServer: http://stackoverflow.com/a/5964334/39182

self.register_listener('before_choose_candidate', self.prompt)
self.server = Server()
self.server.start()
self.server.join()

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 29, 2016

Member

Any particular reason why the server is started and then joined at plugin startup time?

def choice(self, session, task):
artist = ui.input_('Artist:')
realise = ui.input_('Album:')
track = ui.input_('Track:')

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 29, 2016

Member

We should extract these from the task instead of prompting for them.

(Also, realise -> release.)

'realise': realise,
}
url = 'http://musicbrainz.org/taglookup?{0}'.format(urlencode(query))
ui.print_("Choose your tracks and click 'tagger' button to add:")

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 29, 2016

Member

This should probably end in a "." instead of a ":" because there's no prompt here.

_, _, proposal, _ = autotag.tag_album(task.items, search_ids=id_choice)
return proposal
else:
return autotag.tag_item(task.item, search_ids=id_choice)

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 29, 2016

Member

Nice! I'm glad the new API works.

track = ui.input_('Track:')
if not (artist, realise, track):
ui.print_('Please, fill the search query')
return self.prompt

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 29, 2016

Member

Any particular reason a method is returned here?

This comment has been minimized.

Copy link
@tigranl

tigranl Dec 29, 2016

Author Contributor

It's in case if user didn't provide information about track. Anyways, I'm gonna change it and use information from the task.

con.send(data)
if not data:
break
return parse(data)

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 29, 2016

Member

It looks like this Thread doesn't have a run method, which means this code is not actually running in a separate thread. That's fine—this version just blocks the main thread while waiting for a request from the browser—but we should probably just put this code into a plain function instead of a threading.Thread subclass.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 29, 2016

Oh, also, for consistency with other plugins' names, I suggest the name mbweb instead of web_tagger. That also makes it clear that this is MusicBrainz-only.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 30, 2016

Looking great! It also seems that Travis has a few style suggestions, if you're interested in applying those tweaks: https://travis-ci.org/beetbox/beets/jobs/187702547#L848-L854

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2016

Ok. By the way, what should I do with init.py? Do I need to include it in the PR?

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 30, 2016

No, just the main code for the plugin here is fine.

@tigranl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2016

I left untouched two errors:
web_tagger.py:15:80: E501 line too long (85 > 79 characters) web_tagger.py:29:5: F811 redefinition of unused 'urlparse' from line 28
The first one is docstring.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 30, 2016

Great! I took care of those two. (Meanwhile, I don't know what's going on with AppVeyor.)

The HTTP parser thing looks awesome, but the way. It's very short and easy to read.

I think this is close to ready to merge! All we really need now is documentation.

self.host = '127.0.0.1'
self.port = PORT
try: # Start TCP socket, catch soket.error
self.run_server = socket.socket(socket.AF_INET, socket.SOCK_STREAM)

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 30, 2016

Member

I might suggest the name sock or something for the socket, for clarity's sake.

return parse_qs(parsed.query)['id'][0]


class Server():

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 30, 2016

Member

Should probably inherit from object.

.format(urlencode(query))
webbrowser.open(url)
id_choice = self.server.listen()
search_ids.append(id_choice)

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 30, 2016

Member

Hmm; looks like this opens the browser for every track on the album? Maybe we just want to search by album?

This comment has been minimized.

Copy link
@tigranl

tigranl Jan 1, 2017

Author Contributor

But what if there are non-album tracks?

This comment has been minimized.

Copy link
@sampsyo

sampsyo Jan 1, 2017

Member

Hmm, I'm not sure I understand. Weird cases, like missing or extra tracks, would be handled by the ordinary import flow (exactly the same as if you had entered an ID manually).

This comment has been minimized.

Copy link
@tigranl

tigranl Jan 1, 2017

Author Contributor

That's what I was talking about. Okay then.

from beets.ui.commands import PromptChoice
from beets import ui

PORT = 8000

This comment has been minimized.

Copy link
@sampsyo

sampsyo Dec 30, 2016

Member

Move to configuration.

@tigranl
Copy link
Contributor Author

left a comment

Do I need to follow PEP8 on docstrings too? I saw a standard library file with a long docstring (>80) ee79e15

@sampsyo

This comment has been minimized.

Copy link
Member

commented Dec 30, 2016

Yep! Sadly, the standard library isn't all that PEP8-clean. There are lots of old names likeThis, for example, even though PEP8 prescribes this_style.

@tigranl tigranl force-pushed the tigranl:MB_web_interface branch from bb050e2 to 6d4df1b Jan 3, 2017

@tigranl tigranl force-pushed the tigranl:MB_web_interface branch 3 times, most recently from 7073508 to 1f67052 Feb 2, 2017

tigranl added some commits Feb 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.