Conversation
Added empty response check, JSON validation, improved playlist variety, general code cleanup/polishing
Agent-Logs-Url: https://github.com/Blueion76/OctoGen/sessions/ea2c9ab9-e5a8-47cf-ab4b-65734fb157b1 Co-authored-by: Blueion76 <128919662+Blueion76@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Blueion76/OctoGen/sessions/bd8c3c72-3777-4dfc-bc9d-2d3e23d02686 Co-authored-by: Blueion76 <128919662+Blueion76@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Blueion76/OctoGen/sessions/bbc0ced1-290c-4bfe-9e08-5c327b5b9aa4 Co-authored-by: Blueion76 <128919662+Blueion76@users.noreply.github.com>
…ylist-parsing feat: suffix-based parser for ListenBrainz generated playlist names
…ut detection Agent-Logs-Url: https://github.com/Blueion76/OctoGen/sessions/346e335b-f71c-4e5d-87fd-9a4572dd1ddc Co-authored-by: Blueion76 <128919662+Blueion76@users.noreply.github.com>
fix: configurable AI request timeout with retry on timeout errors
There was a problem hiding this comment.
Pull request overview
Adds configurability and resiliency around AI request timeouts, and improves ListenBrainz “Created For You” playlist handling by parsing the generated title suffix into structured metadata.
Changes:
- Add
AI_REQUEST_TIMEOUTconfiguration (env/config models/docs) and plumb it through to the OpenAI-compatible backend; extend retry logic to treat timeouts as retryable. - Add
ListenBrainzAPI.parse_generated_playlist_title()and enrich created-for-you playlist payloads with parsed metadata. - Add unit tests for the ListenBrainz generated-playlist title parser.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
octogen/ai/engine.py |
Add request timeout support for OpenAI-compatible calls; retry on timeouts. |
octogen/main.py |
Load/clamp AI_REQUEST_TIMEOUT and pass into AIRecommendationEngine. |
octogen/config.py |
Add AI_REQUEST_TIMEOUT to the alternate env config loader. |
octogen/models/config_models.py |
Add request_timeout to AIConfig with validation constraints. |
octogen/api/listenbrainz.py |
Add generated playlist title parser + attach parsed metadata to playlist results. |
tests/test_listenbrainz.py |
New unit tests validating title parsing (valid/malformed/mismatch cases). |
README.md |
Document AI_REQUEST_TIMEOUT in the configuration table. |
ENV_VARS.md |
Add full AI_REQUEST_TIMEOUT documentation and update counts. |
.env.example |
Add example AI_REQUEST_TIMEOUT entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "base_url": os.getenv("AI_BASE_URL"), | ||
| "max_context_songs": int(os.getenv("AI_MAX_CONTEXT_SONGS", "500")), | ||
| "max_output_tokens": int(os.getenv("AI_MAX_OUTPUT_TOKENS", "65535")), | ||
| "request_timeout": int(os.getenv("AI_REQUEST_TIMEOUT", "300")), |
There was a problem hiding this comment.
AI_REQUEST_TIMEOUT is documented/configured with a minimum of 30s (and AIConfig.request_timeout enforces ge=30), but this env loader allows values <30. This creates inconsistent behavior vs OctoGenEngine._load_config_from_env() (which clamps to 30) and can cause validation failures depending on which config path is used. Clamp to at least 30 here as well (or centralize the parsing) to keep config handling consistent.
| "request_timeout": int(os.getenv("AI_REQUEST_TIMEOUT", "300")), | |
| "request_timeout": max(30, int(os.getenv("AI_REQUEST_TIMEOUT", "300"))), |
| """Tests for ListenBrainz generated playlist title parser.""" | ||
|
|
||
| from octogen.api.listenbrainz import ListenBrainzAPI | ||
|
|
||
|
|
||
| parse = ListenBrainzAPI.parse_generated_playlist_title |
There was a problem hiding this comment.
This test module is written in pytest style (plain assert statements and pytest-style test discovery), but the repo doesn’t appear to include pytest in requirements.txt or any other documented/dev dependency. Add pytest as a (dev) dependency and/or document how to run the test suite so these tests can actually be executed in CI/local setups.
|
@copilot apply changes based on the comments in this thread |
|
@Blueion76 I can’t apply these changes because the pull request’s branch is the repository’s default branch, which I’m not allowed to push to directly. You can ask me to create a new pull request with the requested changes instead and I'll open a separate PR on a new branch. |
|
@copilot go ahead and make a new PR. |
|
@Blueion76 I've opened a new pull request, #39, to work on those changes. Once the pull request is ready, I'll request review from you. |
No description provided.