-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
[ML] Add mixed cluster tests for inference #108392
Conversation
Pinging @elastic/ml-core (Team:ML) |
...ools-internal/src/main/java/org/elasticsearch/gradle/internal/RestrictedBuildApiService.java
Outdated
Show resolved
Hide resolved
.../javaRestTest/java/org/elasticsearch/xpack/inference/qa/mixed/AzureOpenAiServiceMixedIT.java
Outdated
Show resolved
Hide resolved
...ools-internal/src/main/java/org/elasticsearch/gradle/internal/RestrictedBuildApiService.java
Outdated
Show resolved
Hide resolved
...ed-cluster/src/javaRestTest/java/org/elasticsearch/xpack/inference/qa/mixed/BaseMixedIT.java
Outdated
Show resolved
Hide resolved
...mixed-cluster/src/javaRestTest/java/org/elasticsearch/xpack/inference/qa/mixed/Clusters.java
Outdated
Show resolved
Hide resolved
...ed-cluster/src/javaRestTest/java/org/elasticsearch/xpack/inference/qa/mixed/BaseMixedIT.java
Outdated
Show resolved
Hide resolved
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.
Looks good, thanks for adding these tests I left some minor comments.
I am confused that the concrete test classes inherit from BaseMixedIT
not MixedClusterSpecIT
as the MixedClusterSpecIT
has the mixed cluster from Clusters.java. Should BaseMixedIT
be using public static ElasticsearchCluster cluster = Clusters.mixedVersionCluster();
...mixed-cluster/src/javaRestTest/java/org/elasticsearch/xpack/inference/qa/mixed/Clusters.java
Outdated
Show resolved
Hide resolved
...pgrade/src/javaRestTest/java/org/elasticsearch/xpack/application/OpenAiServiceUpgradeIT.java
Outdated
Show resolved
Hide resolved
...r/src/javaRestTest/java/org/elasticsearch/xpack/inference/qa/mixed/CohereServiceMixedIT.java
Outdated
Show resolved
Hide resolved
...r/src/javaRestTest/java/org/elasticsearch/xpack/inference/qa/mixed/CohereServiceMixedIT.java
Outdated
Show resolved
Hide resolved
...ed-cluster/src/javaRestTest/java/org/elasticsearch/xpack/inference/qa/mixed/BaseMixedIT.java
Outdated
Show resolved
Hide resolved
Thanks for calling this out! It seems that this hierarchy mixup was hiding some underlying problems. |
This reverts parts of commit fca46fd.
…/elastic/elasticsearch into addMixedClusterTestsForInference
Another use case, since the introduction of serverless, is to test BWC against main in addition to the other released branches. | ||
To do so, specify the `bwc.refspec` remote and branch to use for the BWC build as `origin/main`. | ||
To test against main, you will also need to create a new version in link:./server/src/main/java/org/elasticsearch/Version.java[Version.java], | ||
increment `elasticsearch` in link:./build-tools-internal/version.properties[version.properties], and hard-code the `project.version` for ml-cpp |
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.
Just curious why we need to change the version in ml cpp?
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.
If you increment the elasticsearch version to one that doesn't exist, then the build system will not find the (non-existent) ML-CPP artifact. If, instead, we hardcode the current version of elasticsearch, the existing ML-CPP artifact will be downloaded.
Added mixed cluster tests for the inference service. I copied the integration tests from the Upgrade tests, and I copied the MixedCluster set up from this PR
To run the new mixed cluster tests
this command can be used:
./gradlew ':x-pack:plugin:inference:qa:mixed-cluster:v8.15.0#javaRestTest'
. However, to run the mixed cluster tests against main (rather than a previously released version), the code base must be changed, and this flag must be added to the gradlew command-Dtests.bwc.refspec.8.15=origin/main
. The code changes required to run bwc-mixed-cluster tests against main have been added to TESTING.asciidoc and are:Another use case, since the introduction of serverless, is to test BWC against main in addition to the other released branches. To do so, specify the bwc.refspec remote and branch to use for the BWC build as origin/main. To test against main, you will also need to create a new version in [Version.java](https://github.com/elastic/elasticsearch/pull/server/src/main/java/org/elasticsearch/Version.java), increment elasticsearch in [version.properties](https://github.com/elastic/elasticsearch/pull/build-tools-internal/version.properties), and hard-code the project.version for ml-cpp in [ml/build.gradle](https://github.com/elastic/elasticsearch/pull/x-pack/plugin/ml/build.gradle).
Manual testing
I tested making changes to the serialization
TextEmbeddingResults
and only syntax errors were detected as failures, adding new fields didn't cause failures (I'll have to think more about whether this makes sense or not).I removed
CohereEmbeddingsTaskSettings
from theInferenceNamedWriteablesProvider
and this caused theCohereServiceMixedIT
to fail, which is great!I added a fake field to
CohereEmbeddingServiceSettings StreamInput
and the test failed. I added a new field toCohereEmbeddingServiceSettings toXContent
and the test succeeded. I added a new field toCohereEmbeddingServiceSettings writeTo
and the test succeeded.