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

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

Merged
merged 3 commits into from
Oct 22, 2024
Merged

Conversation

hmtbr
Copy link
Collaborator

@hmtbr hmtbr commented Oct 18, 2024

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

@hmtbr hmtbr marked this pull request as ready for review October 18, 2024 06:50
Signed-off-by: Hiroya Matsubara <[email protected]>
@hmtbr hmtbr requested a review from touma-I October 18, 2024 08:09
@matouma
Copy link

matouma commented Oct 18, 2024

@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).

Copy link

@matouma matouma left a comment

Choose a reason for hiding this comment

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

@hmtbr would be great if you can review my comments and let me know your thoughts on how this should work.

@touma-I
Copy link
Collaborator

touma-I commented Oct 18, 2024

@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
Copy link
Contributor

@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
Copy link

Qiragg commented Oct 19, 2024

@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
Copy link
Collaborator

touma-I commented Oct 20, 2024

@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 ?

Copy link
Collaborator

@touma-I touma-I left a comment

Choose a reason for hiding this comment

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

Need to update pyproject.toml to reflect 0.2.2.dev1. (This will be the work-in-progress tag until it is released to pypi)

Signed-off-by: Hiroya Matsubara <[email protected]>
@hmtbr
Copy link
Collaborator Author

hmtbr commented Oct 21, 2024

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
Copy link
Collaborator

touma-I commented Oct 21, 2024

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
Copy link
Collaborator Author

hmtbr commented Oct 22, 2024

@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.

@hmtbr hmtbr merged commit b297156 into dev Oct 22, 2024
4 checks passed
@hmtbr hmtbr deleted the subdomain-focus branch October 22, 2024 03:29
@touma-I
Copy link
Collaborator

touma-I commented Oct 22, 2024

@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.

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.

5 participants