chroma: set a default timeout of 10 seconds #5898
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
Looks good to me, just need a changelog note. Is there a reason this is a draft? |
|
@snejus Thanks, that's good to hear! I kinda wanted to write a test for it and/or make it configurable. It's OK as a patch for me to run but I'd like it to be configurable for others... |
|
I think very few people would every want to adjust this timeout, probably... I think your adjustment goes in line with this previous PR: #5262, where we added |
9999years
force-pushed
the
chroma-timeout
branch
from
8ce0ad7 to
51a0fde
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a default timeout of 10 seconds to AcoustID API calls in the chroma plugin to prevent the import process from hanging when the AcoustID API fails to respond.
- Adds 10-second timeout to
acoustid.lookup()calls during fingerprint matching - Adds 10-second timeout to
acoustid.submit()calls during fingerprint submission - Updates changelog to document this bug fix
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| beetsplug/chroma.py | Adds timeout parameter to acoustid.lookup() and acoustid.submit() API calls |
| docs/changelog.rst | Documents the addition of HTTP request timeouts for AcoustID lookups |
| @@ -98,7 +98,9 @@ def acoustid_match(log, path): | |||
| fp = fp.decode() | |||
| _fingerprints[path] = fp | |||
| try: | |||
| res = acoustid.lookup(API_KEY, fp, duration, meta="recordings releases") | |||
| res = acoustid.lookup( | |||
| API_KEY, fp, duration, meta="recordings releases", timeout=10 | |||
There was a problem hiding this comment.
The hardcoded timeout value of 10 seconds should be extracted as a module-level constant to improve maintainability and consistency, since it's used in multiple places.
| API_KEY, fp, duration, meta="recordings releases", timeout=10 | |
| API_KEY, fp, duration, meta="recordings releases", timeout=ACOUSTID_TIMEOUT |
Copilot uses AI. Check for mistakes.
| @@ -292,7 +294,7 @@ def submit_chunk(): | |||
| """Submit the current accumulated fingerprint data.""" | |||
| log.info("submitting {0} fingerprints", len(data)) | |||
| try: | |||
| acoustid.submit(API_KEY, userkey, data) | |||
| acoustid.submit(API_KEY, userkey, data, timeout=10) | |||
There was a problem hiding this comment.
The hardcoded timeout value of 10 seconds should be extracted as a module-level constant to improve maintainability and consistency, since it's used in multiple places.
| acoustid.submit(API_KEY, userkey, data, timeout=10) | |
| acoustid.submit(API_KEY, userkey, data, timeout=ACOUSTID_SUBMIT_TIMEOUT) |
Copilot uses AI. Check for mistakes.
TODO: Configurable timeouts :)
9999years
force-pushed
the
chroma-timeout
branch
from
51a0fde to
a677eb5
Compare
Description
This prevents the
chromaplugin from hanging the entire import process if the AcoustID API fails to respond.TODO: Configurable timeouts
To Do
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)