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

[embedart] ValueError: could not convert string to float: #1241

Closed
k-f- opened this Issue Jan 18, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@k-f-

k-f- commented Jan 18, 2015

Testing out beets for the first time before fixing my large music library.
Running it over some local files to see how the different plugins react, etc...

error output here: https://gist.github.com/k-f-/78e5ee77101da1d57257

@k-f-

This comment has been minimized.

k-f- commented Jan 18, 2015

@sampsyo sampsyo added the needinfo label Jan 18, 2015

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jan 18, 2015

Thanks for the report. Can you please send along a reproducible test case? I think this may be sensitive to the specific files you're importing, so we will need them to figure out what's going on.

Meanwhile, @Kraymer, any clues about why parsing the ImageMagick output might fail? Perhaps we should catch any unexpected output and log an error.

@k-f-

This comment has been minimized.

k-f- commented Jan 18, 2015

As you requested some files I was testing over that gave errors.
http://kfring.com/downloads/beetsfiles.zip

The command I was running:
beet -v import {FILES} 2>&1 | tee beets.log

Not exactly sure why, but disabling fetchart results in no errors.

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jan 19, 2015

Thanks for providing the samples. Unfortunately, I can't reproduce this; using your config, these files imported without incident. I'm guessing this could be due to different versions of ImageMagick.

I'm going to improve the error-handling around the errors that you're seeing. This should (a) let the process continue even when IM output can't be parsed, and (b) maybe give some more insight into the malformed output.

sampsyo added a commit that referenced this issue Jan 19, 2015

embedart: Do not use shell for subprocess
This avoids bugs when the filename contains spaces, etc.

Pinging @Kraymer: I gave this a brief test, but can you check whether I messed
anything up more subtly?

Came up when looking at #1241.
@sampsyo

This comment has been minimized.

Member

sampsyo commented Jan 19, 2015

OK, error handling added. Can you try this again with the latest source and see if anything is different? If things still go wrong, can you provide the verbose output from an example?

@brunal

This comment has been minimized.

Collaborator

brunal commented Jan 19, 2015

The handling seems broken: if you get a ValueError you're logging it and continuing, but phash_diff won't be defined! Fix & test cases incoming.

brunal added a commit that referenced this issue Jan 19, 2015

embedart: fix behaviour on IM unparseable output, add tests
Test all EmbedCoverArtPlugin.check_art_similarity() code paths.

Improve #1241.
@sampsyo

This comment has been minimized.

Member

sampsyo commented Jan 19, 2015

Oops; that was incredibly dumb of me—thanks, @brunal.

@k-f-

This comment has been minimized.

k-f- commented Jan 20, 2015

@sampsyo, just wanted to let you guys know making this change in my config has stopped this error. Not a fix, but it's a start.

embedart:
    compare_threshold:    0  # disabled. previously 80.

Also, I'd like to just say that beets is wonderful, wonderful, wonderful. I've had a anally retentive music collection for over a decade and probably tried ~10 programs & various scripts/code with surprisingly good results. However, beets is murdering all previous workflows.

🏆
thanks!

edit
embedart seems to be all good with the above change. beets-check however still explodes pretty often. I've opened an issue over there about it over at: geigerzaehler/beets-check#14

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jan 20, 2015

Yep, that disables the comparison feature where this issue arose. I'll close this now since we have better error handling.

And awesome! I'm glad beets is working out well for you.

@sampsyo sampsyo closed this Jan 20, 2015

@k-f-

This comment has been minimized.

k-f- commented Jan 20, 2015

I think what was causing it to explode:
cover.jpg already existed prior to import.
beets fetchart created cover.1.jpg or in some cases cover.2.jpg which I then manually deleted.

I'll take a look at the source code, but perhaps it was user error and the imagemagick stuff was correctly looking for the beets generated .jpg. because it wants to use what the library thinks is the art file.

I had hoped fetchart would not download if cover.jpg existed in the album dir.

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jan 20, 2015

Aha, that is a reasonable explanation.

You are right that fetchart should detect your local file before fetching anything new. Let us know if you find any evidence for why that went wrong.

geigerzaehler added a commit that referenced this issue Feb 1, 2015

Extend item.write() to embed images without changing item
Fixes #1241 and geigerzaehler/beets-check#14

Before, `embed_item` would add the images to the item and then call
`item.write()` to write the item data, including the image, to the
file. This would trigger `item.store()` in the `after_write` hook of
the check plugin. This would in turn try to persist the temporary
`images` attribute of the item, resulting in an SQL error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment