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

chore: Follow-up Documentation cleanup #159

Merged
merged 66 commits into from
Oct 4, 2024
Merged

Conversation

KavithaSiva
Copy link
Contributor

@KavithaSiva KavithaSiva commented Sep 20, 2024

Context

Follow-up of comments not addressed in:

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 September 20, 2024 14:20
@KavithaSiva KavithaSiva changed the title chore: add local testing instructions chore: Follow-up Documentation cleanup Sep 20, 2024
Copy link
Contributor

@marikaner marikaner left a comment

Choose a reason for hiding this comment

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

I left some comments for improvements and I think the local testing section is incorrect (check the comments).
I would also like to add one general section on deployment resolution, so that the relation between model name and deployment ID is clear.

README.md Outdated Show resolved Hide resolved
packages/ai-api/README.md Outdated Show resolved Hide resolved
packages/foundation-models/README.md Outdated Show resolved Hide resolved
packages/langchain/README.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
packages/orchestration/README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ZhongpinWang ZhongpinWang left a comment

Choose a reason for hiding this comment

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

Small improvements suggested

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
packages/ai-api/README.md Outdated Show resolved Hide resolved
SAP AI Core manages access to generative AI models through the global AI scenario `foundation-models`.
Creating a deployment for a model requires access to this scenario.

Each model, model version, and resource group allows for a one-time deployment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Each model, model version, and resource group allows for a one-time deployment.
Deployment requires model name, model version and resource group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Subtle differences between the two, so I will use the original sentence, as it talks about relationship rather than what is required by the API.

packages/foundation-models/README.md Outdated Show resolved Hide resolved
SAP AI Core manages access to generative AI models through the global AI scenario `foundation-models`.
Creating a deployment for a model requires access to this scenario.

Each model, model version, and resource group allows for a one-time deployment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before, a bit confusing to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

Comment on lines +44 to +52
## Relationship between Orchestration and Resource Groups

SAP AI Core manages access to orchestration of generative AI models through the global AI scenario `orchestration`.
Creating a deployment for enabling orchestration capabilities requires access to this scenario.

[Resource groups](https://help.sap.com/docs/sap-ai-core/sap-ai-core-service-guide/resource-groups?q=resource+group) represent a virtual collection of related resources within the scope of one SAP AI Core tenant.
Each resource group allows for a one-time orchestration deployment.

Consequently, each orchestration deployment uniquely maps to a resource group within the `orchestration` scenario.
Copy link
Contributor

Choose a reason for hiding this comment

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

[pp] I would suggest we extract this section into a separate file as it has been repeated many times. It will be very hard to maintain so many packages. At least we should do this definitely in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As, the deployment requirements change between orchestration and foundation-models. I would prefer to write about it in the dedicated client document instead of a separate file, as this would be more confusing.

deekshas8 and others added 2 commits October 2, 2024 16:35
Co-authored-by: Zhongpin Wang <[email protected]>
Copy link
Contributor

@deekshas8 deekshas8 left a comment

Choose a reason for hiding this comment

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

Looks much better now. Please fix the conflicts and Zhongpin's comments, but I am approving anyway.

@KavithaSiva KavithaSiva merged commit 863af52 into main Oct 4, 2024
9 checks passed
@KavithaSiva KavithaSiva deleted the documentation/minor-cleanup branch October 4, 2024 07:57
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