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

feat: (Grounding) Initial client classes with generated API and model #317

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

Conversation

newtork
Copy link
Contributor

@newtork newtork commented Jan 31, 2025

Context

https://github.com/SAP/ai-sdk-java-backlog/issues/160

Generated model and API classes for grounding services.

Feature scope:

  • generated grounding client (unreleased)
    • pipelines
    • vector
    • retrieval
  • unit test

API for Grounding

The PR suggests the following API to interact with Grounding:

// Access client class
GroundingClient client = new GroundingClient();
GroundingClient client = new GroundingClient(new AiCoreService());

// Access generated API classes
PipelinesApi PIPELINES_API = client.pipelines();
VectorApi VECTOR_API = client.vecor();
RetrievalApi RETRIEVAL_API = client.retrieval();

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

@newtork newtork changed the title feat: [Grounding] Initial client classes with generated API and model feat: (Grounding) Initial client classes with generated API and model Jan 31, 2025
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.

generally lgtm, I assume we do all the other release-relevant things once we actually set this to be released.

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.

LGTM so far 👍🏻

if we set this to released, we'd also need some minimal E2E test, sample code, docs & release note. Do you want to do this in a separate PR? (if so, maybe set this to unreleased for now so that we don't release it by accident 😉 )

For E2E tests / sample code, I'd consider at least a GET request. Maybe you can also copy the JS test where they cover creating and deleting a collection == repository. If so, please do that in the dedicated RG for Java.

@newtork newtork self-assigned this Feb 13, 2025
@newtork newtork added the please-review Request to review a pull-request label Feb 14, 2025
Comment on lines +34 to +35

// we don't have testable data yet, but the endpoint works without errors
Copy link
Contributor

Choose a reason for hiding this comment

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

When will we get data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a backlog item for that.
JS colleagues also have no test yet.

https://github.com/SAP/ai-sdk-java-backlog/issues/202

}
}

void testCollectionsGetAll() {
Copy link
Contributor

Choose a reason for hiding this comment

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

When will this be tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's tested as part of the big test suite testCreateDeleteCollection().

If you prefer, I'm okay with testing it as additional dedicated @Test case.

"response": {
"status": 200,
"headers": {
"Content-Type": "application/json"
Copy link
Contributor

Choose a reason for hiding this comment

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

how does the unit testing work?

Copy link
Contributor Author

@newtork newtork Feb 21, 2025

Choose a reason for hiding this comment

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

WireMock supports file-based stubbing

With that, you can define {"reqest: ": {...}, "response": {...}} in JSON format.
(+) less verbose JSON in unit test code
(-) payload decoupled from unit test cases

I wanted to see it in comparison with our other tests. We used the approach in Cloud SDK in some cases.
I'm fine with changing from this implicit to our regular explicit format, if requested.

@CharlesDuboisSAP CharlesDuboisSAP removed the please-review Request to review a pull-request label Feb 20, 2025
@newtork newtork added the please-review Request to review a pull-request label Feb 21, 2025
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.

5 participants