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: Orchestration Grounding convenience #293

Merged
merged 14 commits into from
Jan 27, 2025
Merged

feat: Orchestration Grounding convenience #293

merged 14 commits into from
Jan 27, 2025

Conversation

CharlesDuboisSAP
Copy link
Contributor

@CharlesDuboisSAP CharlesDuboisSAP commented Jan 23, 2025

Context

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

TheGrounding configuration is full of boilerplate code. I added a layer of convenience.

Feature scope:

  • Created Grounding convenience

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

@CharlesDuboisSAP CharlesDuboisSAP self-assigned this Jan 23, 2025
@CharlesDuboisSAP CharlesDuboisSAP changed the title Orchestration Grounding convenience feat: Orchestration Grounding convenience Jan 23, 2025
@CharlesDuboisSAP CharlesDuboisSAP added the please-review Request to review a pull-request label Jan 23, 2025
@Accessors(fluent = true)
public class Grounding implements GroundingProvider {

private String id = "someID";
Copy link
Contributor

Choose a reason for hiding this comment

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

(Major)

Please make this a required argument. I doubt we can use someID globally.

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 should be useless, I set it to empty string

@CharlesDuboisSAP CharlesDuboisSAP enabled auto-merge (squash) January 23, 2025 12:36
@CharlesDuboisSAP CharlesDuboisSAP removed the please-review Request to review a pull-request label Jan 23, 2025
@CharlesDuboisSAP CharlesDuboisSAP added the please-review Request to review a pull-request label Jan 24, 2025
@CharlesDuboisSAP CharlesDuboisSAP enabled auto-merge (squash) January 24, 2025 11:53
// optional filter for document chunks
var databaseFilter =
DocumentGroundingFilter.create()
.id("")
Copy link
Contributor

Choose a reason for hiding this comment

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

(Minor/Comment)

According to spec this identifier shall be unique per request.
Multiple filters are possible per requset.

See spec
GroundingModuleConfig:
  type: object
  required:
    - type
    - config
  additionalProperties: false
  properties:
    type:
      ...
    config:
      type: object
      required:
        - input_params
        - output_param
      additionalProperties: false
      properties:
        filters:
          type: array
          items:
            oneOf:
              - $ref: "#/components/schemas/DocumentGroundingFilter"
          description: Document grounding service filters to be used
        ...

DocumentGroundingFilter:
  type: object
  required:
    - id
    - data_repository_type
  additionalProperties: false
  properties:
    id:
      $ref: "#/components/schemas/GroundingFilterId"

...

GroundingFilterId:
  title: Id
  description: Identifier of this SearchFilter - unique per request.

GroundingFilterSearchConfiguration:
  ...

With the proposed convenience API we are

  • limiting users to define exactly one filter - this is why "id" seems unnecessary.
  • sending a filter no matter what - is this a requirement, that at least one filter must be present?
  • not blocking future changes to cardinality - which is good. Looks like we could easily extend the logic.

Copy link
Contributor

@newtork newtork Jan 24, 2025

Choose a reason for hiding this comment

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

Therefore:
Would allow for setting multiple filter on Grounding then for 1..*?
Or will you keep the cardinality 1?
Is 0 filters an option we have to consider?

Copy link
Contributor Author

@CharlesDuboisSAP CharlesDuboisSAP Jan 24, 2025

Choose a reason for hiding this comment

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

I modified the API to allow 1+ filters.
0 filters is not allowed because we have to specify the repository type

Comment on lines 250 to 258
// optional filter for document chunks
val databaseFilter =
DocumentGroundingFilter.create()
.id("")
.dataRepositoryType(DataRepositoryType.VECTOR)
.searchConfig(GroundingFilterSearchConfiguration.create().maxChunkCount(1))
.documentMetadata(List.of(documentMetadata));

val groundingConfig = Grounding.create().filter(databaseFilter);
Copy link
Contributor

@newtork newtork Jan 24, 2025

Choose a reason for hiding this comment

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

(Comment)

I was looking into other API options where we could increment a number for id implicitly, but they weren't really promising:

Using handler
Suggested change
// optional filter for document chunks
val databaseFilter =
DocumentGroundingFilter.create()
.id("")
.dataRepositoryType(DataRepositoryType.VECTOR)
.searchConfig(GroundingFilterSearchConfiguration.create().maxChunkCount(1))
.documentMetadata(List.of(documentMetadata));
val groundingConfig = Grounding.create().filter(databaseFilter);
// optional filter for document chunks
val groundingConfig =
Grounding.create()
.filterForType(DataRepositoryType.VECTOR)
.apply(filter -> filter
.searchConfig(GroundingFilterSearchConfiguration.create().maxChunkCount(1))
.documentMetadata(List.of(documentMetadata)));
Using return state
Suggested change
// optional filter for document chunks
val databaseFilter =
DocumentGroundingFilter.create()
.id("")
.dataRepositoryType(DataRepositoryType.VECTOR)
.searchConfig(GroundingFilterSearchConfiguration.create().maxChunkCount(1))
.documentMetadata(List.of(documentMetadata));
val groundingConfig = Grounding.create().filter(databaseFilter);
// optional filter for document chunks
val groundingConfig = Grounding.create();
groundingConfig
.filterForType(DataRepositoryType.VECTOR)
.searchConfig(GroundingFilterSearchConfiguration.create().maxChunkCount(1))
.documentMetadata(List.of(documentMetadata));

Copy link
Contributor Author

@CharlesDuboisSAP CharlesDuboisSAP Jan 24, 2025

Choose a reason for hiding this comment

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

This is a filter for 1 request, the id can be empty

@CharlesDuboisSAP CharlesDuboisSAP merged commit 8c86e91 into main Jan 27, 2025
6 checks passed
@CharlesDuboisSAP CharlesDuboisSAP deleted the grounding branch January 27, 2025 10:50
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.

2 participants