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

Fixed a bug that does not get a space type from index setting when its value is empty for compatibility. #2302

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 29 additions & 20 deletions .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ on:
- 'jni/**'
- 'micro-benchmarks/**'
- '.github/workflows/CI.yml'
env:
ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true

jobs:
Get-CI-Image-Tag:
Expand All @@ -42,7 +40,7 @@ jobs:
Build-k-NN-Linux:
strategy:
matrix:
java: [11, 17, 21]
java: [21]

name: Build and Test k-NN Plugin on Linux
runs-on: ubuntu-latest
Expand All @@ -52,11 +50,14 @@ jobs:
# this image tag is subject to change as more dependencies and updates will arrive over time
image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }}
# need to switch to root so that github actions can install runner binary on container without permission issues.
options: --user root
options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }}

steps:
- name: Run start commands
run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }}

- name: Checkout k-NN
uses: actions/checkout@v1
uses: actions/checkout@v4

# Setup git user so that patches for native libraries can be applied and committed
- name: Setup git user
Expand All @@ -65,41 +66,46 @@ jobs:
su `id -un 1000` -c 'git config --global user.email "github-actions[bot]@users.noreply.github.com"'

- name: Setup Java ${{ matrix.java }}
uses: actions/setup-java@v1
uses: actions/setup-java@v4
with:
java-version: ${{ matrix.java }}
distribution: 'temurin'

- name: Run build
# switching the user, as OpenSearch cluster can only be started as root/Administrator on linux-deb/linux-rpm/windows-zip.
run: |
chown -R 1000:1000 `pwd`
if lscpu | grep -i avx2
if lscpu | grep -i avx512f | grep -i avx512cd | grep -i avx512vl | grep -i avx512dq | grep -i avx512bw
then
echo "avx2 available on system"
echo "avx512 available on system"
su `id -un 1000` -c "whoami && java -version && ./gradlew build -Dnproc.count=`nproc`"
elif lscpu | grep -i avx2
then
echo "avx2 available on system"
su `id -un 1000` -c "whoami && java -version && ./gradlew build -Dnproc.count=`nproc` -Davx512.enabled=false"
else
echo "avx2 not available on system"
su `id -un 1000` -c "whoami && java -version && ./gradlew build -Dsimd.enabled=false -Dnproc.count=`nproc`"
echo "avx512 and avx2 not available on system"
su `id -un 1000` -c "whoami && java -version && ./gradlew build -Davx2.enabled=false -Davx512.enabled=false -Dnproc.count=`nproc`"
fi


- name: Upload Coverage Report
uses: codecov/codecov-action@v1
uses: codecov/codecov-action@v4
with:
token: ${{ secrets.CODECOV_TOKEN }}

Build-k-NN-MacOS:
strategy:
matrix:
java: [ 11, 17, 21 ]
java: [ 21 ]

name: Build and Test k-NN Plugin on MacOS
needs: Get-CI-Image-Tag
runs-on: macos-12
runs-on: macos-13

steps:
- name: Checkout k-NN
uses: actions/checkout@v1
uses: actions/checkout@v4

# Setup git user so that patches for native libraries can be applied and committed
- name: Setup git user
Expand All @@ -108,12 +114,14 @@ jobs:
git config --global user.email "github-actions[bot]@users.noreply.github.com"

- name: Setup Java ${{ matrix.java }}
uses: actions/setup-java@v1
uses: actions/setup-java@v4
with:
java-version: ${{ matrix.java }}
distribution: 'temurin'

- name: Install dependencies on macos
run: |
brew install libomp
brew reinstall gcc
export FC=/usr/local/Cellar/gcc/12.2.0/bin/gfortran

Expand All @@ -126,21 +134,21 @@ jobs:
./gradlew build -Dnproc.count=3
else
echo "avx2 not available on system"
./gradlew build -Dsimd.enabled=false -Dnproc.count=3
./gradlew build -Davx2.enabled=false -Davx512.enabled=false -Dnproc.count=3
fi

Build-k-NN-Windows:
strategy:
matrix:
java: [ 11, 17, 21 ]
java: [ 21 ]

name: Build and Test k-NN Plugin on Windows
needs: Get-CI-Image-Tag
runs-on: windows-latest

steps:
- name: Checkout k-NN
uses: actions/checkout@v1
uses: actions/checkout@v4

# Setup git user so that patches for native libraries can be applied and committed
- name: Setup git user
Expand All @@ -149,9 +157,10 @@ jobs:
git config --global user.email "github-actions[bot]@users.noreply.github.com"

- name: Setup Java ${{ matrix.java }}
uses: actions/setup-java@v1
uses: actions/setup-java@v4
with:
java-version: ${{ matrix.java }}
distribution: 'temurin'

- name: Install MinGW Using Scoop
run: |
Expand Down Expand Up @@ -187,4 +196,4 @@ jobs:
# TODO: Detect processor count and set the value of nproc.count
- name: Run build
run: |
./gradlew.bat build -D'simd.enabled=false' -D'nproc.count=4'
./gradlew.bat build -D'avx2.enabled=false' -D'avx512.enabled=false' -D'nproc.count=4'
24 changes: 18 additions & 6 deletions .github/workflows/test_security.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ on:
- 'gradle/**'
- 'jni/**'
- '.github/workflows/test_security.yml'
env:
ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION: true

jobs:
Get-CI-Image-Tag:
Expand All @@ -50,24 +48,38 @@ jobs:
# this image tag is subject to change as more dependencies and updates will arrive over time
image: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-version-linux }}
# need to switch to root so that github actions can install runner binary on container without permission issues.
options: --user root
options: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-options }}

steps:
- name: Run start commands
run: ${{ needs.Get-CI-Image-Tag.outputs.ci-image-start-command }}
- name: Checkout k-NN
uses: actions/checkout@v1
uses: actions/checkout@v4
# Setup git user so that patches for native libraries can be applied and committed
- name: Setup git user
run: |
su `id -un 1000` -c 'git config --global user.name "github-actions[bot]"'
su `id -un 1000` -c 'git config --global user.email "github-actions[bot]@users.noreply.github.com"'

- name: Setup Java ${{ matrix.java }}
uses: actions/setup-java@v1
uses: actions/setup-java@v4
with:
java-version: ${{ matrix.java }}
distribution: 'temurin'

- name: Run build
# switching the user, as OpenSearch cluster can only be started as root/Administrator on linux-deb/linux-rpm/windows-zip.
run: |
chown -R 1000:1000 `pwd`
su `id -un 1000` -c "whoami && java -version && ./gradlew integTest -Dsecurity.enabled=true -Dsimd.enabled=true -Dnproc.count=`nproc`"
if lscpu | grep -i avx512f | grep -i avx512cd | grep -i avx512vl | grep -i avx512dq | grep -i avx512bw
then
echo "avx512 available on system"
su `id -un 1000` -c "whoami && java -version && ./gradlew build -Dnproc.count=`nproc`"
elif lscpu | grep -i avx2
then
echo "avx2 available on system"
su `id -un 1000` -c "whoami && java -version && ./gradlew build -Dnproc.count=`nproc` -Davx512.enabled=false"
else
echo "avx512 and avx2 not available on system"
su `id -un 1000` -c "whoami && java -version && ./gradlew build -Davx2.enabled=false -Davx512.enabled=false -Dnproc.count=`nproc`"
fi
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public SpaceType resolveSpaceType(
SpaceType topLevelSpaceType = getSpaceTypeFromString(topLevelSpaceTypeString);

if (isSpaceTypeConfigured(methodSpaceType) == false && isSpaceTypeConfigured(topLevelSpaceType) == false) {
return getSpaceTypeFromVectorDataType(vectorDataType);
return SpaceType.UNDEFINED;
}

if (isSpaceTypeConfigured(methodSpaceType) == false) {
Expand Down Expand Up @@ -75,13 +75,6 @@ private SpaceType getSpaceTypeFromMethodContext(final KNNMethodContext knnMethod
return knnMethodContext.getSpaceType();
}

private SpaceType getSpaceTypeFromVectorDataType(final VectorDataType vectorDataType) {
if (vectorDataType == VectorDataType.BINARY) {
return SpaceType.DEFAULT_BINARY;
}
return SpaceType.DEFAULT;
}

private SpaceType getSpaceTypeFromString(final String spaceType) {
if (Strings.isEmpty(spaceType)) {
return SpaceType.UNDEFINED;
Expand All @@ -93,4 +86,15 @@ private SpaceType getSpaceTypeFromString(final String spaceType) {
private boolean isSpaceTypeConfigured(final SpaceType spaceType) {
return spaceType != null && spaceType != SpaceType.UNDEFINED;
}

public SpaceType pickDefaultSpaceTypeWhenEmpty(SpaceType resolvedSpaceType, VectorDataType vectorDataType) {
if (resolvedSpaceType != SpaceType.UNDEFINED) {
return resolvedSpaceType;
} else {
if (vectorDataType == VectorDataType.BINARY) {
return SpaceType.DEFAULT_BINARY;
}
return SpaceType.DEFAULT;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -375,14 +375,17 @@ public Mapper.Builder<?> parse(String name, Map<String, Object> node, ParserCont
// If the original knnMethodContext is not null, resolve its space type and engine from the rest of the
// configuration. This is consistent with the existing behavior for space type in 2.16 where we modify the
// parsed value
SpaceType resolvedSpaceType = SpaceTypeResolver.INSTANCE.resolveSpaceType(
final SpaceType resolvedSpaceType = SpaceTypeResolver.INSTANCE.resolveSpaceType(
builder.originalParameters.getKnnMethodContext(),
builder.vectorDataType.get(),
builder.topLevelSpaceType.get()
);
setSpaceType(builder.originalParameters.getKnnMethodContext(), resolvedSpaceType);
setSpaceType(
builder.originalParameters.getKnnMethodContext(),
SpaceTypeResolver.INSTANCE.pickDefaultSpaceTypeWhenEmpty(resolvedSpaceType, builder.vectorDataType.get())
);
validateSpaceType(builder);
resolveKNNMethodComponents(builder, parserContext, resolvedSpaceType);
resolveKNNMethodComponents(builder, parserContext, resolvedSpaceType, builder.vectorDataType.get());
validateFromKNNMethod(builder);
}

Expand Down Expand Up @@ -473,9 +476,10 @@ private void validateCompressionAndModeNotSet(KNNVectorFieldMapper.Builder build
}

private void resolveKNNMethodComponents(
KNNVectorFieldMapper.Builder builder,
Builder builder,
ParserContext parserContext,
SpaceType resolvedSpaceType
SpaceType resolvedSpaceType,
VectorDataType vectorDataType
) {
// Setup the initial configuration that is used to help resolve parameters.
builder.setKnnMethodConfigContext(
Expand Down Expand Up @@ -509,7 +513,7 @@ private void resolveKNNMethodComponents(
builder.originalParameters.getResolvedKnnMethodContext(),
builder.knnMethodConfigContext,
false,
resolvedSpaceType
SpaceTypeResolver.INSTANCE.pickDefaultSpaceTypeWhenEmpty(resolvedSpaceType, vectorDataType)
);

// The original parameters stores both the resolveMethodContext as well as the original provided by the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,7 @@ static KNNMethodContext createKNNMethodContextFromLegacy(
SpaceType topLevelSpaceType
) {
// If top level spaceType is set then use that spaceType otherwise default to spaceType from index-settings
final SpaceType finalSpaceToSet = topLevelSpaceType != SpaceType.UNDEFINED
? topLevelSpaceType
: KNNVectorFieldMapperUtil.getSpaceType(indexSettings);
final SpaceType finalSpaceToSet = topLevelSpaceType != SpaceType.UNDEFINED ? topLevelSpaceType : getSpaceType(indexSettings);
return new KNNMethodContext(
KNNEngine.NMSLIB,
finalSpaceToSet,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ private TrainingModelRequest createTransportRequest(RestRequest restRequest) thr
vectorDataType,
topLevelSpaceType.getValue()
);
resolvedSpaceType = SpaceTypeResolver.INSTANCE.pickDefaultSpaceTypeWhenEmpty(resolvedSpaceType, vectorDataType);
setSpaceType(knnMethodContext, resolvedSpaceType);
TrainingModelRequest trainingModelRequest = new TrainingModelRequest(
modelId,
Expand Down
Loading
Loading