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

fix: move playwright to thread to avoid event loop conflicts #353

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

ErikBjare
Copy link
Owner

@ErikBjare ErikBjare commented Dec 18, 2024

Fixes #351 by moving Playwright to a separate thread to isolate its event loop from prompt_toolkit.

This prevents the asyncio.run() error that occurred when trying to use prompt_toolkit after browser operations.

Changes:

  • Created thread-based browser manager that keeps browser instance alive between operations
  • Updated all browser operations (read_url, search, screenshot) to use the thread
  • Added proper timeout and error handling
  • Added browser readiness check

Tested and confirmed working with:

  • read_url
  • search (both Google and DuckDuckGo)
  • screenshot_url
  • prompt_toolkit input between operations

Important

Moves Playwright operations to a separate thread using BrowserThread to prevent event loop conflicts with prompt_toolkit.

  • Behavior:
    • Moves Playwright operations to a separate thread using BrowserThread to prevent event loop conflicts with prompt_toolkit.
    • Adds timeout and error handling for browser operations.
    • Ensures browser readiness before operations.
  • Implementation:
    • Introduces BrowserThread class in _browser_thread.py to manage browser operations in a separate thread.
    • Updates read_url, search_google, search_duckduckgo, and screenshot_url in _browser_playwright.py to use BrowserThread.
  • Testing:
    • Confirms functionality with read_url, search (Google and DuckDuckGo), and screenshot_url.
    • Verifies prompt_toolkit input works between operations.

This description was created by Ellipsis for 5673f4d. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to ee322c1 in 1 minute and 22 seconds

More details
  • Looked at 290 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. gptme/tools/_browser_thread.py:71
  • Draft comment:
    Ensure that browser is defined before attempting to close it. If sync_playwright().start() or playwright.chromium.launch() fails, browser will be undefined, leading to a potential UnboundLocalError. Consider initializing browser to None before the try block and checking if it's not None before calling close().
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_Q9EY13Rw3y4icFq7


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

global _browser
if _browser is None:
logger.info("Starting browser thread")
_browser = BrowserThread()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding error handling for the BrowserThread initialization in get_browser(). If BrowserThread fails to start, it might raise an exception, which should be caught and logged appropriately.

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 52.50000% with 57 lines in your changes missing coverage. Please review.

Project coverage is 71.03%. Comparing base (0ecf045) to head (5673f4d).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
gptme/tools/_browser_thread.py 51.25% 39 Missing ⚠️
gptme/tools/_browser_playwright.py 55.00% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
- Coverage   72.09%   71.03%   -1.06%     
==========================================
  Files          67       68       +1     
  Lines        5132     5224      +92     
==========================================
+ Hits         3700     3711      +11     
- Misses       1432     1513      +81     
Flag Coverage Δ
anthropic/claude-3-haiku-20240307 69.27% <52.50%> (-1.03%) ⬇️
openai/gpt-4o-mini 68.01% <52.50%> (-1.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ErikBjare ErikBjare force-pushed the dev/fix-playwright-thread branch from ee322c1 to 5673f4d Compare December 18, 2024 15:21
Resolves #351 by moving Playwright to a separate thread to isolate its event loop
from prompt_toolkit. This prevents the asyncio.run() error that occurred when
trying to use prompt_toolkit after browser operations.

Changes:
- Created thread-based browser manager
- Updated all browser operations to use the thread
- Added proper timeout and error handling

Co-authored-by: Bob <[email protected]>
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 5673f4d in 51 seconds

More details
  • Looked at 290 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_VaODCnWXYLf5uYTZ


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

gptme/tools/_browser_playwright.py Show resolved Hide resolved
gptme/tools/_browser_thread.py Show resolved Hide resolved
@ErikBjare
Copy link
Owner Author

ErikBjare commented Dec 18, 2024

Crazy how this entire thing was done by Bob!

Just gave him the issue reference, had to question his choice of process/thread/eventloop, and then off he went!

@ErikBjare ErikBjare merged commit 803d6e5 into master Dec 18, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fails to use browser tool with prompt_toolkit
2 participants