Skip rebuild from scratch after cmake is ran #1636

jmazanec15 · 2024-04-19T22:40:23Z

Description

By commiting the patch changes (via git am instead of git apply), this commit prevents the libraries to be built from scratch every time cmake is ran. This will save additional build time.

from cmake, this looks like:

-- Applying custom patches.
Applying: Add IDGrouper for HNSW
Using index info to reconstruct a base tree...
M	faiss/CMakeLists.txt
M	faiss/Index.h
M	faiss/IndexHNSW.cpp
M	faiss/IndexIDMap.cpp
M	faiss/IndexIDMap.h
M	faiss/impl/HNSW.cpp
M	faiss/impl/ResultHandler.h
M	tests/CMakeLists.txt
Falling back to patching base and 3-way merge...
Auto-merging tests/CMakeLists.txt
Auto-merging faiss/impl/HNSW.cpp
Auto-merging faiss/IndexIDMap.cpp
Auto-merging faiss/Index.h
No changes -- Patch already applied.
Applying: Enable precomp table to be shared ivfpq
Using index info to reconstruct a base tree...
M	faiss/IndexIVFPQ.cpp
M	faiss/IndexIVFPQ.h
M	faiss/IndexIVFPQFastScan.cpp
M	faiss/IndexIVFPQFastScan.h
M	tests/CMakeLists.txt
M	tests/test_disable_pq_sdc_tables.cpp
Falling back to patching base and 3-way merge...
Auto-merging tests/CMakeLists.txt
No changes -- Patch already applied.

In addition to this, this commit separates out the different functionality to make the build system more readable. We can continue to iterate on this in the future.

Issues Resolved

#1552

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.

navneet1v · 2024-04-20T00:01:16Z

@jmazanec15 something seems to go wrong with BWC.. Not sure if it is because of this change

jmazanec15 · 2024-04-21T15:00:12Z

@navneet1v Windows was a GHA failure to setup Java. The BWC seem more related to this change. Taking a look

jmazanec15 · 2024-04-22T15:30:38Z

.gitignore

@@ -17,9 +17,7 @@ oss/*
 jni/CMakeCache.txt
 jni/CMakeFiles
 jni/Makefile
-jni/cmake*
-jni/CPack*


We already removed CPack in #1620

jmazanec15 · 2024-04-23T16:09:31Z

.github/workflows/CI.yml

@@ -38,19 +38,10 @@ jobs:
        with:
          submodules: true

-      # Git functionality in CMAKE file does not work with given ubuntu image. Therefore, handling it here.


This is no longer issue so I removed. All patches should be specified directly in build system.

naveentatikonda · 2024-04-23T17:40:39Z

jni/CMakeLists.txt

-        endif()
-    endif()
-
+    include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/init-faiss.cmake)
    find_package(OpenMP REQUIRED)


Shouldn't we move this to init script like for other packages?
find_package(OpenMP REQUIRED)

Good question - I held off here because we directly link the shared library (opensearch_faiss) against openmp here - so I figured it was good to keep in main cmake because its related to opensearch_faiss

makes sense. Thanks!

naveentatikonda · 2024-04-23T17:43:38Z

jni/cmake/init-faiss.cmake

+set(FAISS_ENABLE_PYTHON OFF)
+
+if(NOT DEFINED SIMD_ENABLED)
+    set(SIMD_ENABLED true)   # set default value as true if the argument is not set


Have you verified if it is passing the argument properly for SIMD_ENABLED after these refactoring changes?

yes, include directive will just basically copy and paste the code into the section its included. See https://cmake.org/cmake/help/latest/command/include.html.

naveentatikonda · 2024-04-23T17:45:17Z

.github/workflows/CI.yml

-      # Git functionality in CMAKE file does not work with given ubuntu image. Therefore, handling it here.
-      - name: Apply Git Patch
-        # Deleting file at the end to skip `git apply` inside CMAKE file
+      - name: Setup git user


Can you add one comment for setting up a git user just for reference?

By commiting the patch changes, this commit prevents the libraries to be built from scratch every time cmake is ran. This will save additional build time. In addition to this, this commit separates out the different functionality to make the build system more readable. We can continue to iterate on this in the future. Signed-off-by: John Mazanec <[email protected]>

By commiting the patch changes, this commit prevents the libraries to be built from scratch every time cmake is ran. This will save additional build time. In addition to this, this commit separates out the different functionality to make the build system more readable. We can continue to iterate on this in the future. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit c35f6ad)

jmazanec15 requested review from heemin32, navneet1v, VijayanB, vamshin, naveentatikonda, junqiu-lei, martin-gaievski and ryanbogan as code owners April 19, 2024 22:40

jmazanec15 added the Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label Apr 19, 2024

jmazanec15 force-pushed the issue-1552 branch from cbfcf38 to d2dd5a9 Compare April 19, 2024 22:42

jmazanec15 force-pushed the issue-1552 branch from d2dd5a9 to 4cb563c Compare April 22, 2024 15:29

jmazanec15 commented Apr 22, 2024

View reviewed changes

jmazanec15 force-pushed the issue-1552 branch 16 times, most recently from 6243eba to d4dbc51 Compare April 23, 2024 15:54

jmazanec15 force-pushed the issue-1552 branch 2 times, most recently from 5e01d7b to 698b4f3 Compare April 23, 2024 16:03

jmazanec15 commented Apr 23, 2024

View reviewed changes

jmazanec15 force-pushed the issue-1552 branch 2 times, most recently from ba91446 to eb062ae Compare April 23, 2024 16:15

jmazanec15 added the backport 2.x label Apr 23, 2024

navneet1v previously approved these changes Apr 23, 2024

View reviewed changes

naveentatikonda reviewed Apr 23, 2024

View reviewed changes

jmazanec15 requested a review from naveentatikonda April 23, 2024 17:45

naveentatikonda previously approved these changes Apr 23, 2024

View reviewed changes

jmazanec15 dismissed stale reviews from naveentatikonda and navneet1v via 9fc70eb April 23, 2024 17:54

jmazanec15 force-pushed the issue-1552 branch from eb062ae to 9fc70eb Compare April 23, 2024 17:54

naveentatikonda approved these changes Apr 23, 2024

View reviewed changes

jmazanec15 requested a review from navneet1v April 23, 2024 19:40

navneet1v approved these changes Apr 23, 2024

View reviewed changes

jmazanec15 merged commit c35f6ad into opensearch-project:main Apr 23, 2024
52 checks passed

opensearch-trigger-bot bot mentioned this pull request Apr 23, 2024

[Backport 2.x] Skip rebuild from scratch after cmake is ran #1648

Merged

jmazanec15 mentioned this pull request Apr 30, 2024

Ensure blas is statically linked #1665

Merged

1 task

junqiu-lei mentioned this pull request May 2, 2024

Update DEVELOPER_GUIDE.md for M Chip Mac #1682

Merged

1 task

heemin32 mentioned this pull request Jul 16, 2024

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

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip rebuild from scratch after cmake is ran #1636

Skip rebuild from scratch after cmake is ran #1636

jmazanec15 commented Apr 19, 2024 •

edited

Loading

navneet1v commented Apr 20, 2024

jmazanec15 commented Apr 21, 2024

jmazanec15 Apr 22, 2024

jmazanec15 Apr 23, 2024

naveentatikonda Apr 23, 2024

jmazanec15 Apr 23, 2024

naveentatikonda Apr 23, 2024

naveentatikonda Apr 23, 2024

jmazanec15 Apr 23, 2024

naveentatikonda Apr 23, 2024

Skip rebuild from scratch after cmake is ran #1636

Skip rebuild from scratch after cmake is ran #1636

Conversation

jmazanec15 commented Apr 19, 2024 • edited Loading

Description

Issues Resolved

Check List

navneet1v commented Apr 20, 2024

jmazanec15 commented Apr 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 commented Apr 19, 2024 •

edited

Loading