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

fix broken build flag, move build to one directory #2442

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix broken build flag, move build to one directory #2442

wants to merge 3 commits into from

Conversation

sam-herman
Copy link
Contributor

Description

Addresses a bug and house cleaning during build.

  1. Cmake has invalid syntax in if condition causing this message:
CMake Error at cmake/init-faiss.cmake:114 (if):
  if given arguments:

    "AND" "NOT" "" "STREQUAL" ""

  Unknown arguments specified
Call Stack (most recent call first):
  CMakeLists.txt:107 (include)
  1. Move all build artifacts created into a build directory following conventional build standards. Currently there are multiple temporary artifacts generated by cmake which are scattered under jni directory.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • [ x] New functionality includes testing.
  • [x ] New functionality has been documented.
  • [x ] API changes companion pull request created.
  • [ x] Commits are signed per the DCO using --signoff.
  • [ x] Public documentation issue/PR created.

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.

@sam-herman sam-herman force-pushed the fix-syntax-typo-breaking-build branch from 3f12185 to 515c986 Compare January 25, 2025 04:02
@navneet1v navneet1v mentioned this pull request Jan 27, 2025
Closed
5 tasks
@sam-herman
Copy link
Contributor Author

@navneet1v @vamshin @jmazanec15 can someone review this fix? Also, I am not sure why the 2 BWC checks are failing and can't re-run the workflows to get them to see if it's a transient things. Can you help me try re-running those?

@navneet1v
Copy link
Collaborator

@navneet1v @vamshin @jmazanec15 can someone review this fix? Also, I am not sure why the 2 BWC checks are failing and can't re-run the workflows to get them to see if it's a transient things. Can you help me try re-running those?

Triggered a re run.

@navneet1v
Copy link
Collaborator

@sam-herman can you rebase your code from main and then we can trigger a run again. I see some changes have been made in main branch which fixes some issues.

@sam-herman
move build to one directory, fix broken flag
c7c3025 
Signed-off-by: Samuel Herman <[email protected]>
@sam-herman
fix make path
232f840 
Signed-off-by: Samuel Herman <[email protected]>
@sam-herman
changelog update
53943ec 
Signed-off-by: Samuel Herman <[email protected]>
@sam-herman sam-herman force-pushed the fix-syntax-typo-breaking-build branch from 25c356a to 53943ec Compare January 28, 2025 22:34
@sam-herman
Copy link
Contributor Author

@sam-herman can you rebase your code from main and then we can trigger a run again. I see some changes have been made in main branch which fixes some issues.

@navneet1v looks like after the rebase there are even more tests failing, including some of the non-bwc ones. maybe try re-run to check if it's transient?

@navneet1v
Copy link
Collaborator

@sam-herman i think for main branch since 3.0 is in progress there are a lot of breaking changes.

@navneet1v
Copy link
Collaborator

Tagging 3.0 release owner here to know when the main branch will be stable since a lot of prs are struck.

@Vikasht34

jmazanec15
jmazanec15 reviewed Jan 29, 2025
View reviewed changes
jni/cmake/init-faiss.cmake
@@ -111,10 +111,10 @@ endif()
if(NOT DEFINED AVX512_SPR_ENABLED)
# Check if the system is Intel(R) Sapphire Rapids or a newer-generation processor
execute_process(COMMAND bash -c "lscpu | grep -q 'GenuineIntel' && lscpu | grep -i 'avx512_fp16' | grep -i 'avx512_bf16' | grep -i 'avx512_vpopcntdq'" OUTPUT_VARIABLE SPR_FLAGS OUTPUT_STRIP_TRAILING_WHITESPACE)
if (AND NOT "${SPR_FLAGS}" STREQUAL "")
Copy link
Member

Choose a reason for hiding this comment

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

Good catch @sam-herman . @mulugetam Can you check this and make sure this change makes sense?

jmazanec15
jmazanec15 reviewed Jan 29, 2025
View reviewed changes
build.gradle
@@ -333,7 +333,7 @@ task cmakeJniLib(type:Exec) {
workingDir 'jni'
def args = []
args.add("cmake")
args.add(".")
Copy link
Member

Choose a reason for hiding this comment

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

Good update. This makes cleaning some much easier. That being said, in the buildJniLib, instead of changing workingDir, can we just do:

make -Cbuild

I worry if we add "/" we might end up having issues with path resolution on different platforms (unless gradle handles this for us)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The windows build seems to work fine https://github.com/opensearch-project/k-NN/actions/runs/13020810027/job/36325044749?pr=2442

@jmazanec15
Copy link
Member

thanks @sam-herman. main is messed up right now due to lucene upgrade. On other PRs, weve been developing directly on 2.x and backporting to main to unblock (#2438 )

@sam-herman
Copy link
Contributor Author

@navneet1v @Vikasht34 Any updates on fixes to BWC and integ tests?

@Vikasht34
Copy link
Contributor

@sam-herman , Tentative changes to be merged in main by End of Next week .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmazanec15 jmazanec15 jmazanec15 left review comments

@heemin32 heemin32 Awaiting requested review from heemin32 heemin32 is a code owner

@navneet1v navneet1v Awaiting requested review from navneet1v navneet1v is a code owner

@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner

@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner

@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner

@junqiu-lei junqiu-lei Awaiting requested review from junqiu-lei junqiu-lei is a code owner

@martin-gaievski martin-gaievski Awaiting requested review from martin-gaievski martin-gaievski is a code owner

@ryanbogan ryanbogan Awaiting requested review from ryanbogan ryanbogan is a code owner

@luyuncheng luyuncheng Awaiting requested review from luyuncheng luyuncheng is a code owner

@shatejas shatejas Awaiting requested review from shatejas shatejas is a code owner

@0ctopus13prime 0ctopus13prime Awaiting requested review from 0ctopus13prime 0ctopus13prime is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sam-herman @navneet1v @jmazanec15 @Vikasht34