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

E2E Different Resource Group #222

Merged
merged 6 commits into from
Dec 3, 2024
Merged

E2E Different Resource Group #222

merged 6 commits into from
Dec 3, 2024

Conversation

rpanackal
Copy link
Member

@rpanackal rpanackal commented Dec 3, 2024

Context

AI/ai-sdk-java-backlog#123.

Include test to cover using OpenAI or Orchestration service with a different resourceGroup than "default".

Feature scope:

  • Add new endpoints in sample app for filtering completions by resource group
  • Fix bug in using custom destination from OpenAI client
    • All calls to OpenAI service declares a default api version by default (unless explicitly set)

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Aligned changes with the JavaScript SDK
  • Documentation updated
  • Release notes updated

Roshin Rajan Panackal added 4 commits December 3, 2024 13:39
- fix type
- Add tests (1 fail)
…-resource-group

# Conflicts:
#	sample-code/spring-app/src/main/java/com/sap/ai/sdk/app/controllers/DeploymentController.java
#	sample-code/spring-app/src/test/java/com/sap/ai/sdk/app/controllers/OpenAiTest.java
- fix bug on resource group with custom destinaiton on openai
- update tests
@rpanackal rpanackal self-assigned this Dec 3, 2024
@rpanackal rpanackal added the please-review Request to review a pull-request label Dec 3, 2024
verify(exactly(1), postRequestedFor(anyUrl()).withoutQueryParam("api-version"));
verify(
exactly(1),
postRequestedFor(anyUrl()).withQueryParam("api-version", equalTo("2024-02-01")));
Copy link
Contributor

@newtork newtork Dec 3, 2024

Choose a reason for hiding this comment

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

(Explanation)

Without the api-version we experienced resource not found error messages from AI Core.

It is a little surprising that before we verified explicitly this header would be missing (?)

Copy link
Member

Choose a reason for hiding this comment

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

Correct, this was intended. The idea being that it is possible for users to prevent the api version from being set, e.g. for cases where the some model works without api version, or maybe it might be set automatically by long access; or if the key of that header ever changes.

That is why the javadoc of the method explicitly states to call withApiVersion afterwards

But to be fair, the above are rather hypothetical scenarios and i think it may be worthwhile to change it in favor of the more convenient approach of setting this automatically

Comment on lines +160 to +163
<dependency>
<groupId>com.sap.cloud.sdk.cloudplatform</groupId>
<artifactId>cloudplatform-connectivity</artifactId>
</dependency>
Copy link
Contributor

@newtork newtork Dec 3, 2024

Choose a reason for hiding this comment

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

(Major)
Wait, why is this necessary?

Because of this code, it seems to infer HttpDestination implicitly at compilation time.

final var destination =
  new AiCoreService().getInferenceDestination(resourceGroup).forModel(GPT_4O);

Copy link
Member Author

Choose a reason for hiding this comment

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

Without the declaration, the maven dependency plugin with goal "analyze-only" reports "Used undeclared dependencies found"

Copy link
Member

@MatKuhr MatKuhr left a comment

Choose a reason for hiding this comment

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

Adding a small section in the docs for this would be useful, otherwise Lgtm

@MatKuhr MatKuhr merged commit 8527ee6 into main Dec 3, 2024
5 checks passed
@MatKuhr MatKuhr deleted the test/e2e-resource-group branch December 3, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please-review Request to review a pull-request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants