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.

Sign up for GitHub

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

Jump to bottom

Add Findstemmer.cmake to find lib stemmer #10884

Closed
wants to merge 1 commit into from
Closed

Add Findstemmer.cmake to find lib stemmer #10884

wants to merge 1 commit into from

Conversation

PHILO-HE
Copy link
Contributor

@PHILO-HE PHILO-HE commented Aug 29, 2024 •
edited
Loading

This pr adds lib stemmer to setup script and allows cmake to find this installed lib during building velox.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 29, 2024
@netlify Netlify
Copy link

netlify bot commented Aug 29, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit f43dd88
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66dfee554c86580008bc4411

@PHILO-HE PHILO-HE force-pushed the find-stemmer branch 2 times, most recently from 25a58a8 to 9b7389b Compare August 29, 2024 01:52
@jinchengchenghh
Copy link
Contributor

Please update the PR title, drop [VL]

@PHILO-HE PHILO-HE changed the title [VL] Add Findstemmer.cmake to find lib stemmer Add Findstemmer.cmake to find lib stemmer Aug 29, 2024
@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Sep 3, 2024

Please update the PR title, drop [VL]

@jinchengchenghh, thanks for your comment! Just updated.

@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Sep 4, 2024

@assignUser, could you spare some time to review this pr? Thanks!

assignUser
assignUser approved these changes Sep 5, 2024
View reviewed changes
Copy link
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

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

Nice!

@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Sep 6, 2024

@kgpai, could you also take a look? Thanks!

@assignUser assignUser added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Sep 7, 2024
@mbasmanova
Copy link
Contributor

@PHILO-HE There are Fuzzer failures. Would you take a look?

@facebook-github-bot
Copy link
Contributor

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Sep 9, 2024

@PHILO-HE There are Fuzzer failures. Would you take a look?

@mbasmanova, we will look into this issue. Assume it is not relevant to this pr.

@mbasmanova
Copy link
Contributor

This diff is either too old, or sits on an old base revision, making it unsafe to land - try rebasing first.

@PHILO-HE Would you rebase?

@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Sep 9, 2024

@mbasmanova, I just rebased the pr. Thanks!

@facebook-github-bot
Copy link
Contributor

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@PHILO-HE
Copy link
Contributor Author

@mbasmanova, the CI failure is irrelevant, and it was just fixed by 4744994.

@mbasmanova
Copy link
Contributor

@PHILO-HE Would you rebase? Still seeing CI failures.

@PHILO-HE
Copy link
Contributor Author

@PHILO-HE Would you rebase? Still seeing CI failures.

@mbasmanova, just rebased.

@facebook-github-bot
Copy link
Contributor

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mbasmanova merged this pull request in 38f9a1f.

@conbench-facebook conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit 38f9a1f5.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

kgpai pushed a commit to kgpai/velox-1 that referenced this pull request Sep 11, 2024
@facebook-github-bot
Back out "Add Findstemmer.cmake to find lib stemmer"
dc49a87 
Summary:
Reverting PR facebookincubator#10884 due to it breaking PyVelox builds and Fuzzer .
Fixes issue : facebookincubator#10963

Original commit changeset: dec13d672129

Original Phabricator Diff: D62373662

Reviewed By: amitkdutta

Differential Revision: D62477240
@kgpai kgpai mentioned this pull request Sep 11, 2024
Closed
kgpai pushed a commit to kgpai/velox-1 that referenced this pull request Sep 11, 2024
@facebook-github-bot
Back out "Add Findstemmer.cmake to find lib stemmer" (facebookincubat…
a12c4ec 
…or#10965)

Summary:
Pull Request resolved: facebookincubator#10965

Reverting PR facebookincubator#10884 due to it breaking PyVelox builds and Fuzzer .
Fixes issue : facebookincubator#10963

Original commit changeset: dec13d672129

Original Phabricator Diff: D62373662

Reviewed By: xiaoxmeng, amitkdutta, kagamiori

Differential Revision: D62477240
facebook-github-bot pushed a commit that referenced this pull request Sep 11, 2024
@facebook-github-bot
Back out "Add Findstemmer.cmake to find lib stemmer" (#10965)
485329e 
Summary:
Pull Request resolved: #10965

Reverting PR #10884 due to it breaking PyVelox builds and Fuzzer .
Fixes issue : #10963

Original commit changeset: dec13d672129

Original Phabricator Diff: D62373662

Reviewed By: xiaoxmeng, amitkdutta, kagamiori

Differential Revision: D62477240

fbshipit-source-id: a98174e1be031864e84dd11ba8eb3475415d5a0e
@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by 485329e.

@kgpai
Copy link
Contributor

kgpai commented Sep 12, 2024

@PHILO-HE We had to revert this PR because it broke pyvelox and fuzzer builds. It's because the docker images produced did not work well with pyvelox . I recommend we create another PR just to make changes to the images, see if they work and subsequently land them and then re-add this findstemmer change. Thank you !

@PHILO-HE
Copy link
Contributor Author

@PHILO-HE We had to revert this PR because it broke pyvelox and fuzzer builds. It's because the docker images produced did not work well with pyvelox . I recommend we create another PR just to make changes to the images, see if they work and subsequently land them and then re-add this findstemmer change. Thank you !

@kgpai, I understand the reverting. Will fix that issue.

PHILO-HE added a commit to PHILO-HE/velox that referenced this pull request Sep 12, 2024
@PHILO-HE
Add Findstemmer.cmake to find lib stemmer (facebookincubator#10884)
2288f22 
Summary:
This pr adds lib stemmer to setup script and allows cmake to find this installed lib during building velox.

Pull Request resolved: facebookincubator#10884

Reviewed By: Yuhta

Differential Revision: D62373662

Pulled By: mbasmanova

fbshipit-source-id: dec13d67212901cf98a96534e090cd54b8ff0331
PHILO-HE added a commit to PHILO-HE/velox that referenced this pull request Sep 13, 2024
@PHILO-HE
Add Findstemmer.cmake to find lib stemmer (facebookincubator#10884)
9790090 
Summary:
This pr adds lib stemmer to setup script and allows cmake to find this installed lib during building velox.

Pull Request resolved: facebookincubator#10884

Reviewed By: Yuhta

Differential Revision: D62373662

Pulled By: mbasmanova

fbshipit-source-id: dec13d67212901cf98a96534e090cd54b8ff0331
athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
@PHILO-HE @athmaja-n
Add Findstemmer.cmake to find lib stemmer (facebookincubator#10884)
aaee74a 
Summary:
This pr adds lib stemmer to setup script and allows cmake to find this installed lib during building velox.

Pull Request resolved: facebookincubator#10884

Reviewed By: Yuhta

Differential Revision: D62373662

Pulled By: mbasmanova

fbshipit-source-id: dec13d67212901cf98a96534e090cd54b8ff0331
athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
@athmaja-n
Back out "Add Findstemmer.cmake to find lib stemmer" (facebookincubat…
bfcc286 
…or#10965)

Summary:
Pull Request resolved: facebookincubator#10965

Reverting PR facebookincubator#10884 due to it breaking PyVelox builds and Fuzzer .
Fixes issue : facebookincubator#10963

Original commit changeset: dec13d672129

Original Phabricator Diff: D62373662

Reviewed By: xiaoxmeng, amitkdutta, kagamiori

Differential Revision: D62477240

fbshipit-source-id: a98174e1be031864e84dd11ba8eb3475415d5a0e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@assignUser assignUser assignUser approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall Reverted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@PHILO-HE @jinchengchenghh @mbasmanova @facebook-github-bot @kgpai @assignUser