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: RAGe client #300

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

feat: RAGe client #300

wants to merge 25 commits into from

Conversation

KavithaSiva
Copy link
Contributor

@KavithaSiva KavithaSiva commented Nov 18, 2024

Context

AI/gen-ai-hub-sdk-js-backlog#176.

This PR adds a dedicated package for generated rage client. The vector and retrieval clients are also E2E tested for a scenario of creating a document collection and querying the created collection.
The package is currently set to private, as we do not plan to release it yet.

The generated clients have been manually modified for now, to target the correct API endpoint.
This workaround would be removed after https://github.com/SAP/cloud-sdk-backlog/issues/1259.

The most relevant changes to be reviewed are in sample-code, tests/e2e-tests.

Definition of Done

  • Code is tested (Unit, E2E)
  • [] Error handling created / updated & covered by the tests above
  • [ ] Documentation updated
  • [ ] (Optional) Aligned changes with the Java SDK
  • [ ] (Optional) Release notes updated

@KavithaSiva KavithaSiva marked this pull request as draft November 18, 2024 10:46
@KavithaSiva KavithaSiva marked this pull request as ready for review November 28, 2024 10:29
@KavithaSiva KavithaSiva changed the title feat: Rage client feat: RAGe client Nov 28, 2024
Copy link
Contributor

@jjtang1985 jjtang1985 left a comment

Choose a reason for hiding this comment

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

LGTM, as a PoC.

Comment on lines 1 to 3
export * from './client/pipeline/pipeline-api/index.js';
export * as VectorApi from './client/vector/vector-api/index.js';
export * as RetrievalApi from './client/retrieval/retrieval-api/index.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

By checking the log of the public API check, I did not see anything for pipeline api.
I would assume it's because of the alias of vector api and retrival api.
After merging the spec, it should be solved.

Copy link
Contributor

@shibeshduw shibeshduw Nov 28, 2024

Choose a reason for hiding this comment

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

[q] What is the reason for the folder structure to be pipeline/pipeline-api instead of just pipeline-api/ ? We should follow the same naming convention as the other packages i.e. client/**/schema. That way, all generated files will get ignored by the public api check because we are passing this regex in the GH action.

Or alternatively you can also add another regex for nesting folder structure. That would fix the public api check fail. But i would say we maintain a consistent structure across packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason we need an additional nested structure is because we also always clear the output folder every time we generate a client and I wanted to ensure that the correct client gets deleted. Deletion is required so that we have a clean slate, and get to know if there are any breaking changes in the client during generation.

The current script looks like:

+ openapi-generator --generateESM --clearOutputDir -i ./src/spec/vector-api.yaml -o ./src/client/vector
- openapi-generator --generateESM --clearOutputDir -i ./src/spec/vector-api.yaml -o ./src/client/

As the generator always creates a new folder under the output directory specified, if we don't have a nested structure and use just /src/client/, all clients in the /src/client/ folder will get cleared.

Copy link
Member

@tomfrenken tomfrenken Nov 28, 2024

Choose a reason for hiding this comment

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

An alternative would be to change the generator script to move the folder after generation, then the folder structure would look as expected, however, then we would need to chain a few more commands together, e.g.:
rm -rf ./src/client/vector-api && pnpm generate:vector && mv ./src/client/vector/vector-api ./src/client && rm ./src/client/vector, at that point it makes more sense to update the ignore pattern, I agree

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 would assume it's because of the alias of vector api and retrival api.
After merging the spec, it should be solved.

Aliases, are needed as there are duplicates in the spec.
But, aliases are currently not considered in the public API check.

I can selectively export only some of the API, schema types from each client to work around this problem for now.

Copy link
Contributor Author

@KavithaSiva KavithaSiva Nov 28, 2024

Choose a reason for hiding this comment

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

@jjtang1985 The public API check GH action currently doesn't support aliases while using exports.
To enable the build to succeed, I now, selectively exported only some types and API that I use for my E2E test.

If the spec isn't merged soon, and we need aliases to export API and all types successfully, we might then have to work on considering them in the public API check.

Also, note before releasing the client, NOT all types are exported from the Vector and Search API now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@KavithaSiva , we can come back again, when the merged spec is provided.
As discussed, we should start working on the generator and stop working on this branch for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants