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

Add url filtering to Cheerio scraper. Also fix multiple issues of link limit enforcement. #1417

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jitsmaster
Copy link

No description provided.

@jitsmaster jitsmaster changed the title Merge from main repo Add url filtering to Cheerio scraper. Also fix issue of link limit not getting enforced. Dec 20, 2023
@jitsmaster jitsmaster changed the title Add url filtering to Cheerio scraper. Also fix issue of link limit not getting enforced. Add url filtering to Cheerio scraper. Also fix multiple issues of link limit enforcement. Dec 20, 2023
@@ -1,67 +1,66 @@
import axios from 'axios'
Copy link
Contributor

Choose a reason for hiding this comment

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

are you using smtg like formatter that will rearrange the imports by alphabetical order?

Copy link
Author

Choose a reason for hiding this comment

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

It's a VS code extension. If it's causing trouble, I can manually change them back.

Copy link
Contributor

@HenryHengZJ HenryHengZJ Jan 14, 2024

Choose a reason for hiding this comment

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

ah okay, if you can remove the changes that are not part of the actual web scraping changes, that'd be great!

@HenryHengZJ
Copy link
Contributor

many thanks for the PR! Can you highlight some of the cases where the current solution doesnt work/or has limitation? that in this PR we manage to solve that?

@jitsmaster
Copy link
Author

jitsmaster commented Jan 10, 2024

Sorry for my late response.

Thank you very much for spending your valuable time reviewing this PR.

The purpose of this PR is to allow scraping large set of web pages without node out of memory.

All the filtering mechanisms and re-enforcement of max count is for that purpose.

Since we can never know how much memory it will actually take to store all the scraped content, there will be quite a bit of trials and errors to figure out the max size that is acceptable.

My original intent is to use a piece by piece model to store any content scraped right away, but LangChain doesn't support it, and seems to have no intention to adopt this model.

I do plan for a second stage changes based on the current model. That is to store the content on disk instead of memory, and before text split. That will be another PR.

Cheers!

@luc4t
Copy link

luc4t commented May 1, 2024

this would be really helpful, I just read this PR after posting here: #1566 (comment)

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.

3 participants