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

remove screenshot when check user goal #1147

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

LawyZheng
Copy link
Collaborator

@LawyZheng LawyZheng commented Nov 6, 2024

Important

Remove screenshot step when checking user goal by updating check_user_goal_complete and related scraping functions to support optional screenshots.

  • Behavior:
    • check_user_goal_complete in agent.py now refreshes the page without taking a screenshot.
    • scrape_website and scrape_web_unsafe in scraper.py updated to accept with_screenshot parameter.
  • Classes:
    • ScrapedPage in scraper.py now has private attributes _browser_state, _clean_up_func, and _scrape_exclude.
  • Types:
    • Introduced ScrapeExcludeFunc in scraper.py for type clarity.
  • Misc:
    • Updated scrape_exclude type in app.py to ScrapeExcludeFunc.

This description was created by Ellipsis for 8bd282b. 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 8bd282b in 1 minute and 28 seconds

More details
  • Looked at 310 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. skyvern/forge/agent.py:838
  • Draft comment:
    The removal of re-scraping the page before checking the user goal can improve performance by avoiding unnecessary operations. Ensure that scraped_page is up-to-date before this check.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR modifies the agent_step function to remove the re-scraping of the page before checking if the user goal is achieved. This change is reflected in the removal of the new_scraped_page variable and the associated logic. The scraped_page is now directly used in the check_user_goal_complete function. This change seems to be intentional to avoid unnecessary re-scraping, which can improve performance. However, it's important to ensure that the scraped_page is up-to-date before this check. The PR also modifies the check_user_goal_complete function to refresh the scraped_page without taking screenshots, which aligns with the PR title and intent.
2. skyvern/forge/agent.py:910
  • Draft comment:
    Refreshing the scraped_page without screenshots aligns with the PR's intent. Ensure this change doesn't affect the goal-checking process.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The check_user_goal_complete function now refreshes the scraped_page without taking screenshots, which aligns with the PR's intent to remove screenshots during this check. This change should be verified to ensure it doesn't affect the functionality of the goal-checking process.
3. skyvern/webeye/scraper/scraper.py:260
  • Draft comment:
    The scrape_website function now always returns a ScrapedPage, which simplifies error handling in the calling code.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    The scrape_website function signature has been changed to always return a ScrapedPage object instead of potentially returning None. This change ensures that the function always returns a valid object, which can simplify error handling in the calling code.

Workflow ID: wflow_uGnmUTA2MgoTJ72m


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.

@@ -211,6 +212,26 @@ class ScrapedPage(BaseModel):
html: str
extracted_text: str | None = None

_browser_state: BrowserState = PrivateAttr()
Copy link
Contributor

Choose a reason for hiding this comment

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

The _browser_state and _clean_up_func attributes in ScrapedPage appear to duplicate functionality already present in the BrowserState class. Consider using or extending BrowserState for managing browser state and cleanup.

@LawyZheng LawyZheng merged commit b62c2ca into main Nov 6, 2024
2 checks passed
@LawyZheng LawyZheng deleted the lawy/remove-screenshot-when-check-user-goal branch November 6, 2024 15:20
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.

1 participant