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

Implement the basic AcousticBrainz Submit plugin #2342

Merged
merged 10 commits into from
Jan 1, 2017
Merged

Conversation

inytar
Copy link
Contributor

@inytar inytar commented Dec 23, 2016

This is a new upload for #2340 as that had some strange problems with the pull request and trying to fix it closed the pull request.

Copy of the last comment in that pull request:

Thanks for all the advice! I updated the pull request to hopefully fix the strange problem with authorization/ect. I'll also look into the comments, documentation and tests.
Some things I'm wondering:

  • Do we want to raise an error if analysis or submitting a file fails?
  • Do we want to save that a file has been analyzed and submitted? The analysis takes a long time and should not change if done multiple times and as far as I can see the the server code ignores any upload that is exactly the same as one already in the system.

@ghost
Copy link

ghost commented Dec 23, 2016

this failure here https://travis-ci.org/beetbox/beets/jobs/186448711 is the same I'm seeing in my PR. something has changed in either mutagen or the underlying build env.

@ghost
Copy link

ghost commented Dec 24, 2016

it seems to be mutagen. I filed #2343

@sampsyo
Copy link
Member

sampsyo commented Dec 24, 2016

Thanks for opening the new PR! This one seems to have cleared up the weird problems.

Some answers to your questions from the other thread:

Do we want to raise an error if analysis or submitting a file fails?

I think the appropriate thing to do is to use self._log.error to report the error, but no need to stop the whole process. This is usually nice because it means that if things go wrong for one specific file, or if the network connection drops out intermittently, progress isn't lost.

Do we want to save that a file has been analyzed and submitted? The analysis takes a long time and should not change if done multiple times and as far as I can see the the server code ignores any upload that is exactly the same as one already in the system.

Good idea! If you don't mind, let's add this in a subsequent PR—we can merge the first version and then add this "don't submit twice" feature. It should be pretty easy—we can use an abz_submitted flexible attribute or somesuch, or even store the full analysis results—but we can get the basic functionality working first.

A UserError is now raised if the plugin can be started.
If an item fails to be analyzed or be submited an error is logged and
the next item is tried.
@inytar
Copy link
Contributor Author

inytar commented Dec 26, 2016

I've updated this pull request, hopefully worked all the comments from the last pull request and added documentation.

@sampsyo
Copy link
Member

sampsyo commented Dec 26, 2016

Wonderful; thank you! ✨

It looks like Travis is reminding us that the new file needs the standard header, complete with license text and the coding comment at the top.

@sampsyo
Copy link
Member

sampsyo commented Dec 26, 2016

Or! There's some argument to be made here that we should just add this functionality to the acousticbrainz plugin, which has the same requirements (i.e., just requests). That would be neat and tidy!

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

I made a couple of small suggestions. It's definitely time to call this GCI task "completed," though, if you'd like to submit it!

)
else:
# Implicit path to extractor, search for it in path
# TODO how to check for on Windows?
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment still necessary? Any more questions about Windows?

Installation
------------

The `absubmit` plugin requires the the `streaming_extractor_music`_ program to run. Its source can be found on `github`_, and while it is possible to compile the extractor from source, AcousticBrainz would prefer if you used thier binary (see the AcousticBrainz `FAQ`_).
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize "GitHub"

Copy link
Contributor

Choose a reason for hiding this comment

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

the the => the

AcousticBrainz Submit Plugin
============================

The `absubmit` plugin uses the `streaming_extractor_music`_ program to analyse an audio file and calculate different acoustic properties of the audio, the plugin then uploads this metadata to the AcousticBrainz server. The plugin does this when calling the ``beet absumbit [QUERY]`` command or on importing if the `auto` configuration options is set to ``yes``.
Copy link
Member

Choose a reason for hiding this comment

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

The first comma here should actually end the sentence (and begin a new one).

…acoustic properties of the audio. The plugin then uploads…


pip install requests

After installing both the extractor binary and request you can enable the plgin ``absubmit`` in your configuration (see :ref:`using-plugins`).
Copy link
Member

Choose a reason for hiding this comment

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

plgin -> plugin

Copy link
Contributor

Choose a reason for hiding this comment

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

and request => and requests

@SusannaMaria
Copy link
Contributor

Have added force option and check if acousticbrainz tag already set, similar to the acousticbrainz plugin pull request from myself
SusannaMaria@c474142

@SusannaMaria
Copy link
Contributor

There is some problem with removing the temp-json file in windows10.

   File "C:\Python27\lib\site-packages\beets-1.4.3-py2.7.egg\beetsplug\absubmit.py", line 149, in _get_analysis
os.remove(filename)
   WindowsError: [Error 32] Der Prozess kann nicht auf die Datei zugreifen, da sie von einem anderen Prozess verwendet wird: 'c:\\users\\susanna\\appdata\\local\\temp\\tmpem70wo.json'

Sorry the the german error text, it says the file cannot deleted because ifs used from a different process.

Will check with https://docs.python.org/3/library/tempfile.html
it should solve this issue, maybe its completely windows related.

@@ -0,0 +1,31 @@
AcousticBrainz Submit Plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

options is => option is

Configuration
-------------

To configute the plugin, make a ``absubmit:`` section in your configuration file. The available options are:
Copy link
Contributor

Choose a reason for hiding this comment

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

configute => configure

@sampsyo
Copy link
Member

sampsyo commented Dec 27, 2016

Thanks, @SusannaMaria—perhaps we're doing something wrong when closing the new temporary file?

@SusannaMaria
Copy link
Contributor

Have to check tomorrow, just finished some changes because of force-option on @inytar fork (made a pull request on his fork) inytar@b8fa8dc

@inytar
Copy link
Contributor Author

inytar commented Dec 31, 2016

Sorry for the long wait, was away for a couple of days. I've fixed the above mentioned comments.
@sampsyo would you like to submit the plugin as is, or prefer it being merged with the acousticbrainz plugin? The reasons I originally did not do this are: this plugin needs a new dependency that is not needed for the original acousticbrainz plugin; this plugin does not save anything to file, but just upoads it to acousticbrainz.
@SusannaMaria where you albe to fix the temp file problem in your branch?

@sampsyo
Copy link
Member

sampsyo commented Dec 31, 2016

No problem at all! There's no rush here. 😃

Sure—on further reflection, I can see the argument for keeping these as separate plugins. For the other one, the documentation doesn't even need to mention that you need to install the analysis program.

It looks like Travis is still complaining about the coding line at the top of the file—and indeed, the new Python file needs the entire standard header comment. Other than that, I think this is ready to merge.

Installation
------------

The `absubmit` plugin requires the `streaming_extractor_music`_ program to run. Its source can be found on `GitHub`_, and while it is possible to compile the extractor from source, AcousticBrainz would prefer if you used thier binary (see the AcousticBrainz `FAQ`_).
Copy link
Member

Choose a reason for hiding this comment

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

thier -> their

@sampsyo sampsyo merged commit b57b3f7 into beetbox:master Jan 1, 2017
sampsyo added a commit that referenced this pull request Jan 1, 2017
Implement the basic AcousticBrainz Submit plugin
sampsyo added a commit that referenced this pull request Jan 1, 2017
@sampsyo
Copy link
Member

sampsyo commented Jan 1, 2017

This is all merged up! ✨ Thank you for all your effort on this, @inytar.

@tux-00
Copy link
Contributor

tux-00 commented Jan 1, 2017

Maybe I'm wrong but the documentation says "To submit audio to AcousticBrainz, files must be tagged with MusicBrainz Identifiers. If they are not tagged, we recommend Picard.", maybe it's a good idea to write this sentence in docs/plugins/absubmit.rst ?

@sampsyo
Copy link
Member

sampsyo commented Jan 2, 2017

@tux-00 Sure; it's a good point to mention that music needs MBIDs for submission. Would you mind adding that to the docs with a PR? It's just a matter of hitting the "edit" button on the appropriate page on GitHub.

And we clearly shouldn't say "we recommend Picard" because beets of course does this perfectly well. Under ordinary circumstances, users' music will have already had MBIDs added by the import process.

@tux-00
Copy link
Contributor

tux-00 commented Jan 2, 2017

No problem but just to make sure that I understand :

  • What plugin generates the MBID of a track ? absubmit ?

I'm a little bit confused with the names of the beets MusicBrainz plugins. For example the plugin "MusicBrainz Submit Plugin" does not seem to submit anything. It's possible that I don't understand something, I'm new with MusicBrainz !

@sampsyo
Copy link
Member

sampsyo commented Jan 2, 2017

The importer (with no plugin at all) adds track MBIDs.

And that mbsumbit plugin generates text that you can paste into a submission form. That's the best we can do, unfortunately, because MusicBrainz doesn't offer a programmatic API for submission.

And no worries at all! I'm happy to answer questions, even if they seem basic.

@tux-00
Copy link
Contributor

tux-00 commented Jan 2, 2017

When trying to run beets from sources, if the plugin doesn't find the the streaming_extractor_music binary beets raises this error :

Traceback (most recent call last):
  File "/usr/bin/beet", line 9, in <module>
    load_entry_point('beets==1.4.3', 'console_scripts', 'beet')()
  File "/home/tux/git/beets/beets/ui/__init__.py", line 1209, in main
    _raw_main(args)
  File "/home/tux/git/beets/beets/ui/__init__.py", line 1192, in _raw_main
    subcommands, plugins, lib = _setup(options, lib)
  File "/home/tux/git/beets/beets/ui/__init__.py", line 1089, in _setup
    plugins = _load_plugins(config)
  File "/home/tux/git/beets/beets/ui/__init__.py", line 1075, in _load_plugins
    plugins.send("pluginload")
  File "/home/tux/git/beets/beets/plugins.py", line 451, in send
    for handler in event_handlers()[event]:
  File "/home/tux/git/beets/beets/plugins.py", line 434, in event_handlers
    for plugin in find_plugins():
  File "/home/tux/git/beets/beets/plugins.py", line 288, in find_plugins
    _instances[cls] = cls()
  File "/home/tux/git/beets/beetsplug/absubmit.py", line 84, in __init__
    self.extractor = distutils.spawn.find_executable(self.extractor)
AttributeError: 'module' object has no attribute 'spawn'

So if I understand, if the importer cannot find the musicbrainz track id I cannot use the acousticbrainz to tag my tracks.
Is that right ?

@sampsyo
Copy link
Member

sampsyo commented Jan 2, 2017

Hmm; that actually looks like an unrelated bug—I don't know why distutils.spawn can't be found, but this doesn't have to do with MBIDs.

EDIT: Oh, I see—those were two different questions. Right; the importer has to get an MBID for your music in order for AB submission to work.

@sampsyo
Copy link
Member

sampsyo commented Jan 2, 2017

In fact, could you please open a separate bug with that traceback and full details about your setup? Looks like we need to investigate more closely.

@tux-00
Copy link
Contributor

tux-00 commented Jan 2, 2017

@sampsyo yes I will ! :)

@tux-00
Copy link
Contributor

tux-00 commented Jan 3, 2017

It's strange, I can't reproduce the distutils error.. Now I get a good error reporting :
error: Extractor command does not exist: /usr/bin/test.

I remember that I have played with pip to install beets from sources, maybe something happened and reboot solve the problem. This is the only reason I've found !

@sampsyo
Copy link
Member

sampsyo commented Jan 3, 2017

Weird! Well, let us know if it comes back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants