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

Apply custom patch only once by comparing the patch-id of the last commit #1833

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

heemin32
Copy link
Collaborator

@heemin32 heemin32 commented Jul 16, 2024

Description

As more custom patches are added, it fails to re-apply same patches if more than one patch touch the same file. This lead to consistent build failure whenever we build jni library more than once. The workaround with the failure is removing both git rebase-apply folder and external/faiss folder and build from scratch which is inconvenient and increase development time.

To resolve the issue, changed the cmake file to apply custom patch only once by comparing the patch-id of the last commit.

Also, removed submodule: true option in CI as it seems it is not needed anymore after we upgraded linux image for CI build.

Issues Resolved

N/A

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@heemin32
Copy link
Collaborator Author

heemin32 commented Jul 16, 2024

Verifying if removing submodule: true works in CI. If this change works, it is thanks to the #1636.

@heemin32 heemin32 force-pushed the cmake branch 3 times, most recently from 707998d to f84330e Compare July 16, 2024 18:49
@heemin32 heemin32 force-pushed the cmake branch 2 times, most recently from 0662eeb to 4a96b99 Compare July 16, 2024 19:41
@heemin32 heemin32 marked this pull request as ready for review July 16, 2024 20:20
@heemin32 heemin32 added the Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label Jul 16, 2024
@jmazanec15
Copy link
Member

@heemin32 In some cases, we need to pull the submodule from top level (see build script). So I dont think this approach would work with that

junqiu-lei
junqiu-lei previously approved these changes Jul 16, 2024
@heemin32
Copy link
Collaborator Author

heemin32 commented Jul 16, 2024

@heemin32 In some cases, we need to pull the submodule from top level (see build script). So I dont think this approach would work with that

I can edit the build script as same as window.

git submodule update --init -- jni/external/nmslib
git -C jni/external/nmslib apply --3way --ignore-space-change --ignore-whitespace ../../patches/nmslib/0001-Initialize-maxlevel-during-add-from-enterpoint-level.patch
git -C jni/external/nmslib apply --3way --ignore-space-change --ignore-whitespace ../../patches/nmslib/0002-Adds-ability-to-pass-ef-parameter-in-the-query-for-h.patch

git submodule update --init -- jni/external/faiss
git -C jni/external/faiss apply --3way --ignore-space-change --ignore-whitespace ../../patches/faiss/0001-Custom-patch-to-support-multi-vector.patch
git -C jni/external/faiss apply --3way --ignore-space-change --ignore-whitespace ../../patches/faiss/0002-Enable-precomp-table-to-be-shared-ivfpq.patch
git -C jni/external/faiss apply --3way --ignore-space-change --ignore-whitespace ../../patches/faiss/0003-Custom-patch-to-support-range-search-params.patch
git -C jni/external/faiss apply --3way --ignore-space-change --ignore-whitespace ../../patches/faiss/0004-Custom-patch-to-support-binary-vector.patch

ryanbogan
ryanbogan previously approved these changes Jul 16, 2024
junqiu-lei
junqiu-lei previously approved these changes Jul 16, 2024
@heemin32 heemin32 dismissed stale reviews from junqiu-lei and ryanbogan via f257a8d July 16, 2024 23:23
@heemin32 heemin32 force-pushed the cmake branch 2 times, most recently from f257a8d to 5f3562a Compare July 16, 2024 23:23
@heemin32 heemin32 changed the title Apply custom patch only once after submodule is checked out Apply custom patch only once by comparing the last patch id Jul 16, 2024
@heemin32 heemin32 requested review from ryanbogan and junqiu-lei July 16, 2024 23:26
@heemin32
Copy link
Collaborator Author

heemin32 commented Jul 16, 2024

@jmazanec15 raised a concern on having a list of patch file in multiple places and suggested to use git patch-id. Thanks.

@heemin32 heemin32 force-pushed the cmake branch 2 times, most recently from 2227510 to 335b36e Compare July 17, 2024 03:43
@heemin32 heemin32 changed the title Apply custom patch only once by comparing the last patch id Apply custom patch only once by comparing the patch-id of the last commit Jul 17, 2024
@heemin32
Copy link
Collaborator Author

bwc test is failing for all PR now with Exception in thread "main" java.io.FileNotFoundException: /home/runner/work/k-NN/k-NN/qa/restart-upgrade/build/private/artifact_tmp/opensearch-knn-2.16.0.zip (No such file or directory)

Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

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

Thanks thats awesome!

@heemin32 heemin32 merged commit 1b425df into opensearch-project:main Jul 17, 2024
16 of 52 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 17, 2024
heemin32 added a commit that referenced this pull request Jul 17, 2024
…1843)

Signed-off-by: Heemin Kim <[email protected]>
(cherry picked from commit 1b425df)

Co-authored-by: Heemin Kim <[email protected]>
@heemin32 heemin32 deleted the cmake branch July 17, 2024 14:12
naveentatikonda pushed a commit to naveentatikonda/k-NN that referenced this pull request Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants