search

IBM / data-prep-kit

Open source project for data preparation of LLM application builders
https://ibm.github.io/data-prep-kit/
Apache License 2.0
316 stars 135 forks source link

implement subdomain focus feature in data-prep-connector #725

Closed hmtbr closed 1 month ago

hmtbr commented 1 month ago

Why are these changes needed?

If the user provides https://research.example.com/ as a seed url for the data-prep-connector, there is a requirement that the user wants to automatically apply subdomain focus so we do not crawl other subdomains than research for the domain example.com.

This PR implements the subdomain focus feature.

Related issue number (if any).

724

matouma commented 1 month ago

@qiragg Please confirm that you can see this PR and comment on it. You can also tag me once your you approve it. I am also soliciting input from the broader community on this one. I know we did it before in the first part of the year and I want to make sure we capture lessons learned from the previous implementation (what worked and what did not work).

touma-I commented 1 month ago

@qiragg @yuanchi2807 Please review and comment as you see fit. @qqirag it may help if you can elaborate further on the rated issue based on previous experience with similar functionality in bluecrawl

yuanchi2807 commented 1 month ago

@Qiragg @yuanchi2807 Please review and comment as you see fit. @qqirag it may help if you can elaborate further on the rated issue based on previous experience with similar functionality in bluecrawl

I am not knowledgable enough to crawling requirements to make a comment.

Qiragg commented 1 month ago

@Qiragg @yuanchi2807 Please review and comment as you see fit. @qqirag it may help if you can elaborate further on the rated issue based on previous experience with similar functionality in bluecrawl

In bluecrawl, we do provide the ability to do subdomain_focus automatically based on the input seed url but we cannot focus on multiple subdomains per job. For crawling, it would make sense to be able to launch a single job that focuses on multiple subdomains which is what this feature would provide. This is a functionality that was missing in DPK-connector and is much needed for launching certain targeted crawls.

There are a couple of points to discuss here:

  1. It is more intuitive to set the default subdomain_focus to be true but we will not do that here based we do not want to depart from the default crawler behavior in the earlier version.
  2. We expect the user to not provide ibm.com and research.ibm.com as seed urls at the same time if they want subdomain_focus to be applied to on research. It is not clear if we should reject jobs which are improperly configured in such a way or allow the user to rectify their mistake. While this is obvious in this case; in rare cases, the user may want to apply path_focus and subdomain_focus at the same time by providing: research.ibm.com/help/ and ibm.com/support/ as seeds. In such a case, my understanding is that they will end up crawling both ibm.com/help/ and ibm.com/support/.

Ideally, we only want to crawl research.ibm.com/help/ and ibm.com/support/ for such a user-defined case.

The PR looks good to me for now. I think if we get feedback regarding a different design choice that the user wants, we can think about it at that point.

touma-I commented 1 month ago

@Qiragg Thanks for the analysis. Good stuff: Can you click on "Files Changed" then click on the green button "review changes" then check "approve" then click submit to indicate your approval. @hmtbr: 1- what happens if we use the seed url : http://www.research.ibm.com/ and set path focus to true (i.e. path focus is for the Root (i.e. / ), will that produce the same result as sub-domain focus ? 2- Also, what if the user specifies the "unexpected" in the case that @Qiragg identified above (ie. 2 seeds URLs ibm.com and research.ibm.com and subdomain focus set to true? Will the code default to the least restrictive or the more restrictive case ?

hmtbr commented 1 month ago

1- what happens if we use the seed url : http://www.research.ibm.com/ and set path focus to true (i.e. path focus is for the Root (i.e. / ), will that produce the same result as sub-domain focus ?

Currently there is no support for path focus with subdomain. It will produce pages from domain *.ibm.com.

2- Also, what if the user specifies the "unexpected" in the case that @Qiragg identified above (ie. 2 seeds URLs ibm.com and research.ibm.com and subdomain focus set to true? Will the code default to the least restrictive or the more restrictive case ?

It will produce pages from domain *.ibm.com.

touma-I commented 1 month ago

1- what happens if we use the seed url : http://www.research.ibm.com/ and set path focus to true (i.e. path focus is for the Root (i.e. / ), will that produce the same result as sub-domain focus ?

Currently there is no support for path focus with subdomain. It will produce pages from domain *.ibm.com.

@hmtbr @Qiragg @shahrokhDaijavad is this a bug in the logic then that need to be fixed ? If the application specifies www.ibm.com/docs with path focus it should not receive anything from research.ibm.com/docs . No?

hmtbr commented 1 month ago

@touma-I Thanks for your approval. The cases don't produce intuitive results actually. I will address them in another issue and PR soon. This PR itself won't introduce any regression, so I merge this now to unblock our work.

touma-I commented 1 month ago

@touma-I Thanks for your approval. The cases don't produce intuitive results actually. I will address them in another issue and PR soon. This PR itself won't introduce any regression, so I merge this now to unblock our work.

Thanks @hmtbr Agree. Now that this has matured to this point, it is appropriate to consider usability by outsiders who are not intimately familiar with our internal processes. I do think that if path focus is done right, we won't need subdomain focus and It will be appropriate to do a second iteration. thanks again.