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

fetchart: Restore iTunes Store source #2718

Merged
merged 4 commits into from Aug 15, 2018

Conversation

Projects
None yet
2 participants
@nathdwek
Copy link
Member

commented Oct 26, 2017

This is a tentative at addressing #2551 . For the moment this is very much a work in progress, but I would like to discuss some points.

For the moment I took python-itunes from @ocelma, stripped it of some unnecessary stuff and fixed some stuff to make python3 compatible.

Open questions are:

  • Place in the beetbox universe: I do not really see us maintaining this for anyone's else purpose than ours. I would like to strip the code down even more so that it is as as simple and minimalistic as required to fetch the images, nothing else. Keeping it inside beets gives us some more freedom in terms of interface.
  • Licensing/recognition: python-itunes is GPL. I removed all the comments at the top of the file for the moment, but only to be able to remove clutter, for my own sanity when reading the code. I think even outside of licensing issues we should give some recognition to the original author(s).

To fix:

  • Some pep8 and long lines
  • Python2-3 compat: I think key issues will be urllib and strings when dealing with requests/responses
  • Remove a ton of unnecessary getters and unneeded complexity
@sampsyo

This comment has been minimized.

Copy link
Member

commented Oct 28, 2017

Cool! I'm very much in favor of adopting the code ourselves to help get this functionality back into shape. Let's do it!

Trimming the necessary code down to its essence does make it possible to keep things simple. If we can get @ocelma's consent to relicense the code under MIT, let's provide proper attribution and start simplifying down from there. After a quick look, it seems like it shouldn't be too hard to pull out the pieces we need.

On the other hand, if we don't hear back from @ocelma, then publishing a separate module to PyPI would be an acceptable fallback that would keep the GPL implications contained.

@nathdwek nathdwek force-pushed the fix-itunes branch from 87522c4 to 2e32989 Aug 14, 2018

@nathdwek

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

This should be waaaaay simpler.
I think it is ready to merge pending but a test could be added.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

Wow; this does indeed look way better. Awesome!

Could you please update the docs to clarify that this plugin no longer needs a dependency? (A changeling entry would also be rad. 😎)

nathdwek added some commits Aug 14, 2018

fetchart: restore itunes art source
Reimplement minimalistic itunes scraper from scratch
fetchart: fix itunes debug info
Some typos + make it more useful overall

@nathdwek nathdwek force-pushed the fix-itunes branch from 2e32989 to 399bfb9 Aug 15, 2018

@nathdwek

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

I am currently writing the changelog and doc before merging.
What criteria is used to determine itunes should be in the default list of sources (speed is mentioned in the docs, is it because for amazon and albumart the image url is constructed directly without scraping a page/json response?)?

nathdwek added a commit that referenced this pull request Aug 15, 2018

Changelog for #2718
Reverts 49e548b from #2540
Fixes #2251 #2371
@nathdwek

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

I elected to simply revert (some parts of) the fetchart doc to what it was before #2540.
The itunes source was actually still in the default config of the plugin.

Ready to merge I think.

@nathdwek

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

Appveyor/branch fail is a simple timeout of the launching of the whole test run due resource unavailability if I read the message correctly.

Feel free to merge whenever if this suits you, @sampsyo !

Changelog for #2718
Reverts 49e548b from #2540
Fixes #2371 #2551

@nathdwek nathdwek force-pushed the fix-itunes branch from 3a07fee to ff6306b Aug 15, 2018

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

Looks perfect! Thanks for all your effort on this. 🤖 🐟 🌟

Your earlier question about criteria for inclusion as a default source is a good one. Fast-ish is a good idea, but it seems like this source will be pretty fast too. The exact definition of “fast enough” is open to interpretation.

@sampsyo sampsyo merged commit cab6910 into master Aug 15, 2018

0 of 4 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@nathdwek

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

I would like to add a note about iTunes Store rate limiting. The doc says it is limited to about 20calls per minute. Trying by hand it is definitely higher, but I was still able to get rate-limited.

For that reason this provider might start to error out and not provide candidate during a batch import, for which it might not be the ideal provider.

Is this worth mentioning somewhere? What makes me not like this is that the failure is silent unless beets runs in verbose mode, so there could be some extreme case where no cover/a non-perfect is found although one is available on the itunes store and the user is not aware of that.

A possibility would be to check if the http error can be detected for rate-limiting and then issue a warning of some kind.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

Hmm; interesting point. Another alternative would be voluntarily rate limit ourselves—that is, keep track of how much we’re requesting and gradually slow things down. But 3 seconds per call (on average) is pretty slow.

It’s probably worth at least mentioning in the docs. A visible warning may be fine but could get noisy and annoying in the context of a full, interactive import.

@nathdwek

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

Correction: I actually made a typo when mashing my keyboard to test the rate limiting.
I wanted to see what status code would be emitted when triggering the rate limiting using the following:

import requests

while True:
    try:
        r = requests.get('https://itunes.apple.com/search', params={
            'term': 'Pink Floyd Meddle',
            'entity': 'album',
            'media': 'music'
        })
        print(r.status_code)
        r.raise_for_status()
    except requests.RequestException as e:
        print(e)
        break

I can't seem to reach the limit even when letting it loop for 10 minutes straight.

You could try it out on your side, but I think for the moment we can leave it at that.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 15, 2018

Aha; that’s great to know. Let’s leave it here and revisit it if problems crop up.

@arcresu arcresu deleted the fix-itunes branch Apr 24, 2019

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.