feat(chat): add --timeout option to ask command#260
feat(chat): add --timeout option to ask command#260Flosters wants to merge 1 commit intoteng-lin:mainfrom
Conversation
Long or complex prompts can exceed the default HTTP timeout, causing the request to fail silently. Adding --timeout (default: 120s) lets users increase this for slower responses without changing client-wide settings. Usage: notebooklm ask "summarize everything" --timeout 300
📝 WalkthroughWalkthroughA Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable --timeout option for the ask_cmd command in the NotebookLM CLI, allowing users to adjust the request timeout for long prompts. The feedback suggests refining the CLI option by using click.IntRange to ensure positive values and removing redundant default text from the help string. Additionally, it is recommended to implement granular timeouts (separating connection and read timeouts) when initializing the NotebookLMClient to improve network resilience.
| @click.option( | ||
| "--timeout", | ||
| default=120, | ||
| type=int, | ||
| show_default=True, | ||
| help="Request timeout in seconds. Increase for long prompts (default: 120).", | ||
| ) |
There was a problem hiding this comment.
The help text contains a redundant default value since show_default=True is enabled, which will cause Click to append [default: 120] automatically. Additionally, the timeout should be validated to ensure it is a positive value to avoid potential issues with the underlying network client.
| @click.option( | |
| "--timeout", | |
| default=120, | |
| type=int, | |
| show_default=True, | |
| help="Request timeout in seconds. Increase for long prompts (default: 120).", | |
| ) | |
| @click.option( | |
| "--timeout", | |
| default=120, | |
| type=click.IntRange(min=1), | |
| show_default=True, | |
| help="Request timeout in seconds. Increase for long prompts.", | |
| ) |
|
|
||
| async def _run(): | ||
| async with NotebookLMClient(client_auth) as client: | ||
| async with NotebookLMClient(client_auth, timeout=float(timeout)) as client: |
There was a problem hiding this comment.
Following the general rules for network clients, it is recommended to use granular timeouts. A single float value sets the same timeout for connection and reading. Using a shorter connect timeout (e.g., 10s) while allowing a longer read timeout improves resilience by failing fast on connection issues while still accommodating slow responses. Note that the NotebookLMClient type hints should also be updated to support this.
| async with NotebookLMClient(client_auth, timeout=float(timeout)) as client: | |
| async with NotebookLMClient(client_auth, timeout=(10.0, float(timeout))) as client: |
References
- For network clients, use granular timeouts with a shorter connect timeout and longer read/write timeouts to improve resilience by detecting connection issues quickly while accommodating slow responses.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooklm/cli/chat.py`:
- Around line 105-111: The --timeout click option currently uses type=int which
permits 0 and negative values; change its validation to require strictly
positive integers by replacing type=int with click.IntRange(min=1) (or add a
callback that raises click.BadParameter if timeout < 1) in the `@click.option`
declaration for "--timeout" in chat.py so the CLI rejects non-positive values at
parse time and returns a clear validation error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42881f76-bc63-4350-9e68-f8b5eac381d9
📒 Files selected for processing (1)
src/notebooklm/cli/chat.py
| @click.option( | ||
| "--timeout", | ||
| default=120, | ||
| type=int, | ||
| show_default=True, | ||
| help="Request timeout in seconds. Increase for long prompts (default: 120).", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In current httpx versions, are timeout values of 0 or negative numbers valid for connect/read/write/pool timeouts?
💡 Result:
-
httpx.Timeoutitself doesn’t validate ranges forconnect/read/write/pool(it just stores whatever you pass). So0or negative floats can be constructed without error. [1] -
But when actually used for I/O, negative timeout values are not valid:
- On the sync path, HTTPCore uses Python sockets, and
socket.settimeout(value)requires a nonnegative real number (orNone); negative values are invalid. [2] - On the Trio async backend, Trio’s timeout context managers raise
ValueErrorifsecondsis negative. [3]
- On the sync path, HTTPCore uses Python sockets, and
-
0is allowed but means “don’t wait” (e.g., Python sockets treat0as non-blocking mode, i.e. effectively an immediate timeout/no waiting). [2] -
To disable timeouts, use
None(per-timeout, orTimeout(None, ...)). [1]
Answer: 0 is technically allowed (immediate / non-blocking semantics), but negative numbers are not valid in practice and will error in common backends; use None to disable instead. [1][2][3]
Sources:
[1] HTTPX Timeout implementation (httpx/_config.py, master)
[2] Python socket.settimeout() docs (nonnegative real, 0 => non-blocking)
[3] Trio move_on_after() docs (negative seconds => ValueError)
Validate --timeout as strictly positive at parse time.
type=int allows 0 and negative values. Negative timeouts will fail at runtime (Python sockets and Trio backends reject non-negative values), and 0 creates non-blocking semantics without waiting. Reject these early with a CLI validation error for clearer UX.
Proposed fix
`@click.option`(
"--timeout",
default=120,
- type=int,
+ type=click.IntRange(min=1),
show_default=True,
- help="Request timeout in seconds. Increase for long prompts (default: 120).",
+ help="Request timeout in seconds. Increase for long prompts.",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @click.option( | |
| "--timeout", | |
| default=120, | |
| type=int, | |
| show_default=True, | |
| help="Request timeout in seconds. Increase for long prompts (default: 120).", | |
| ) | |
| `@click.option`( | |
| "--timeout", | |
| default=120, | |
| type=click.IntRange(min=1), | |
| show_default=True, | |
| help="Request timeout in seconds. Increase for long prompts.", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooklm/cli/chat.py` around lines 105 - 111, The --timeout click
option currently uses type=int which permits 0 and negative values; change its
validation to require strictly positive integers by replacing type=int with
click.IntRange(min=1) (or add a callback that raises click.BadParameter if
timeout < 1) in the `@click.option` declaration for "--timeout" in chat.py so the
CLI rejects non-positive values at parse time and returns a clear validation
error.
1 participant
Summary
Long or complex prompts can exceed the default HTTP timeout, causing
notebooklm askto fail silently with a connection error. There is currently no way to increase this without changing client-wide settings in code.This PR adds a
--timeoutoption (default: 120s) to theaskcommand, letting users increase it for slower responses.Usage:
Changes
cli/chat.py: add--timeout(int, default 120,show_default=True) toasktimeout=float(timeout)toNotebookLMClient()askinvocation only — no effect on other commandsTest plan
notebooklm ask --helpshows--timeout INTEGERwith default[default: 120]notebooklm ask "question" --timeout 300uses the extended timeoutSummary by CodeRabbit
--timeoutoption to theaskcommand, allowing users to customize request timeout duration (default: 120 seconds).