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

cancel stop loading window #535

Merged
merged 1 commit into from
Jul 2, 2024
Merged

cancel stop loading window #535

merged 1 commit into from
Jul 2, 2024

Conversation

LawyZheng
Copy link
Collaborator

@LawyZheng LawyZheng commented Jul 1, 2024

🚀 This description was created by Ellipsis for commit 4c22bdb

Summary:

Replaced STOPLOADING scrape type with an additional NORMAL scrape type in the scraping retry logic.

Key points:

  • Modified SCRAPE_TYPE_ORDER in skyvern/constants.py to replace ScrapeType.STOPLOADING with ScrapeType.NORMAL.
  • Updated _build_and_record_step_prompt in skyvern/forge/agent.py to retry normal scraping twice before reloading the page.
  • Changed error logging message in _build_and_record_step_prompt to reflect the new retry logic.

Generated with ❤️ by ellipsis.dev

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 4c22bdb in 52 seconds

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

Workflow ID: wflow_lotJw86ZEy8vXwnO


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.

@@ -15,4 +15,4 @@ class ScrapeType(StrEnum):
RELOAD = "reload"


SCRAPE_TYPE_ORDER = [ScrapeType.NORMAL, ScrapeType.STOPLOADING, ScrapeType.RELOAD]
SCRAPE_TYPE_ORDER = [ScrapeType.NORMAL, ScrapeType.NORMAL, ScrapeType.RELOAD]
Copy link
Contributor

Choose a reason for hiding this comment

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

The change to have two ScrapeType.NORMAL entries in SCRAPE_TYPE_ORDER might not be effective for different scraping strategies. Consider reintroducing ScrapeType.STOPLOADING or another distinct strategy to maintain diverse scraping attempts.

Suggested change
SCRAPE_TYPE_ORDER = [ScrapeType.NORMAL, ScrapeType.NORMAL, ScrapeType.RELOAD]
SCRAPE_TYPE_ORDER = [ScrapeType.NORMAL, ScrapeType.STOPLOADING, ScrapeType.RELOAD]

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we keep it? I suspect it stuck the task due to the no timeout for page.evaluate(). Maybe we will be back to use it again when we find a way to time out page.evaluate()

@LawyZheng LawyZheng merged commit 98713b0 into main Jul 2, 2024
2 checks passed
@LawyZheng LawyZheng deleted the cancel-stop-loading branch July 2, 2024 04:33
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.

2 participants