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

refactor: Update Firecrawl to use v1 API #12574

Merged

Conversation

ftonato
Copy link
Contributor

@ftonato ftonato commented Jan 9, 2025

Summary

  • Updated API endpoints from v0 to v1 for scrape and crawl methods.
  • Enhanced error handling in scrape_url and check_crawl_status methods.
  • Introduced helper methods _extract_common_fields and _format_crawl_status_response for better code organization and readability.

Resolves #12571

- Updated API endpoints from v0 to v1 for scrape and crawl methods.
- Enhanced error handling in `scrape_url` and `check_crawl_status` methods.
- Introduced helper methods _extract_common_fields and _format_crawl_status_response for better code organization and readability.

Closes: langgenius#12571
@nickscamara
Copy link
Contributor

From Firecrawl v1 perspective, looks good!

@ftonato ftonato marked this pull request as ready for review January 10, 2025 14:31
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jan 10, 2025
crazywoola
crazywoola previously approved these changes Jan 12, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 12, 2025
@crazywoola
Copy link
Member

Please fix the tests and run dev/reformat to bypass the lint errors.

@ftonato
Copy link
Contributor Author

ftonato commented Jan 13, 2025

Hello @crazywoola,

I’ve made the changes, but I couldn’t run the workflow or the pytest tests in the project directory locally. Could you run the workflow on this PR to check the results?

@ftonato
Copy link
Contributor Author

ftonato commented Jan 13, 2025

UPDATE: I still need to make some changes. I’ll let you know once they’re done.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jan 13, 2025
Copy link
Contributor

@nickscamara nickscamara left a comment

Choose a reason for hiding this comment

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

Might be worth deleting the params that don't exist anymore so it doesn't confuse people. But overall looks good!

@ftonato ftonato requested a review from nickscamara January 14, 2025 18:07
Copy link
Contributor

@nickscamara nickscamara left a comment

Choose a reason for hiding this comment

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

LGMT (from Firecrawl side) - @crazywoola mind taking another look at it and testing e2e to make sure it is good? Thank you!

crazywoola
crazywoola previously approved these changes Jan 22, 2025
@crazywoola
Copy link
Member

Hello it seems it didn't pass the tests.

@ftonato
Copy link
Contributor Author

ftonato commented Jan 22, 2025

Hello it seems it didn't pass the tests.

Hello, I am going to take a look again 🤦


@crazywoola Can you run again the pipeline?

@ftonato ftonato force-pushed the refactor/update-firecrawl-api-to-v1 branch from 390a9f3 to a3b1e6c Compare January 22, 2025 13:32
@crazywoola crazywoola merged commit 6024d8a into langgenius:main Jan 23, 2025
6 checks passed
@crazywoola
Copy link
Member

Merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move Firecrawl from v0 to v1 API
3 participants