-
Notifications
You must be signed in to change notification settings - Fork 132
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
Add changes for AVX-512 support in k-NN. #2110
Changes from 4 commits
a35b564
298e61e
3e44af5
1eabadf
e8e28a2
072e289
bc3a058
8134b5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,11 +86,13 @@ public class KNNSettings { | |
public static final String KNN_FAISS_AVX2_DISABLED = "knn.faiss.avx2.disabled"; | ||
public static final String QUANTIZATION_STATE_CACHE_SIZE_LIMIT = "knn.quantization.cache.size.limit"; | ||
public static final String QUANTIZATION_STATE_CACHE_EXPIRY_TIME_MINUTES = "knn.quantization.cache.expiry.minutes"; | ||
public static final String KNN_FAISS_AVX512_DISABLED = "knn.faiss.avx512.disabled"; | ||
|
||
/** | ||
* Default setting values | ||
*/ | ||
public static final boolean KNN_DEFAULT_FAISS_AVX2_DISABLED_VALUE = false; | ||
public static final boolean KNN_DEFAULT_FAISS_AVX512_DISABLED_VALUE = false; | ||
public static final String INDEX_KNN_DEFAULT_SPACE_TYPE = "l2"; | ||
public static final String INDEX_KNN_DEFAULT_SPACE_TYPE_FOR_BINARY = "hamming"; | ||
public static final Integer INDEX_KNN_DEFAULT_ALGO_PARAM_M = 16; | ||
|
@@ -302,6 +304,12 @@ public class KNNSettings { | |
Dynamic | ||
); | ||
|
||
public static final Setting<Boolean> KNN_FAISS_AVX512_DISABLED_SETTING = Setting.boolSetting( | ||
KNN_FAISS_AVX512_DISABLED, | ||
KNN_DEFAULT_FAISS_AVX512_DISABLED_VALUE, | ||
NodeScope | ||
); | ||
|
||
/** | ||
* Dynamic settings | ||
*/ | ||
|
@@ -429,6 +437,10 @@ private Setting<?> getSetting(String key) { | |
return KNN_FAISS_AVX2_DISABLED_SETTING; | ||
} | ||
|
||
if (KNN_FAISS_AVX512_DISABLED.equals(key)) { | ||
return KNN_FAISS_AVX512_DISABLED_SETTING; | ||
} | ||
|
||
if (KNN_VECTOR_STREAMING_MEMORY_LIMIT_IN_MB.equals(key)) { | ||
return KNN_VECTOR_STREAMING_MEMORY_LIMIT_PCT_SETTING; | ||
} | ||
|
@@ -460,6 +472,7 @@ public List<Setting<?>> getSettings() { | |
ADVANCED_FILTERED_EXACT_SEARCH_THRESHOLD_SETTING, | ||
KNN_FAISS_AVX2_DISABLED_SETTING, | ||
KNN_VECTOR_STREAMING_MEMORY_LIMIT_PCT_SETTING, | ||
KNN_FAISS_AVX512_DISABLED_SETTING, | ||
QUANTIZATION_STATE_CACHE_SIZE_LIMIT_SETTING, | ||
QUANTIZATION_STATE_CACHE_EXPIRY_TIME_MINUTES_SETTING | ||
); | ||
|
@@ -499,6 +512,22 @@ public static boolean isFaissAVX2Disabled() { | |
} | ||
} | ||
|
||
public static boolean isFaissAVX512Disabled() { | ||
try { | ||
return KNNSettings.state().getSettingValue(KNNSettings.KNN_FAISS_AVX512_DISABLED); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do proper null checks here? In general, I think its best to avoid catching all exceptions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like java boolean cannot be null. So null check won't be possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akashsha1 as users will manually set this setting in opensearch.yml, there is a chance of getting null. To avoid it shall we change it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. spoke with Naveen on slack, and updated to |
||
} catch (Exception e) { | ||
// In some UTs we identified that cluster setting is not set properly an leads to NPE. This check will avoid | ||
// those cases and will still return the default value. | ||
log.warn( | ||
"Unable to get setting value {} from cluster settings. Using default value as {}", | ||
KNN_FAISS_AVX512_DISABLED, | ||
KNN_DEFAULT_FAISS_AVX512_DISABLED_VALUE, | ||
e | ||
); | ||
return KNN_DEFAULT_FAISS_AVX512_DISABLED_VALUE; | ||
} | ||
} | ||
|
||
public static Integer getFilteredExactSearchThreshold(final String indexName) { | ||
return KNNSettings.state().clusterService.state() | ||
.getMetadata() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldnt these be mutually exclusive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we want simd (avx2, avx512 - highest of whichever is present) to be enabled by default. The order of checks would be:
if (AVX512 enabled and present) { use avx512 }
else if (AVX2 enabled and present) { use avx2 }
else { use generic version }
by making it mutually exclusive, the intermediate step cannot be achieved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Jack's point is to update it as something like this
./gradlew build -Davx2.enabled=false -Davx512.enabled=true
as we can't use both of them at the same time