-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
There was a problem hiding this 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.
Co-authored-by: Marika Marszalkowski <[email protected]>
Co-authored-by: Marika Marszalkowski <[email protected]>
Co-authored-by: Marika Marszalkowski <[email protected]>
Co-authored-by: Marika Marszalkowski <[email protected]>
Co-authored-by: Marika Marszalkowski <[email protected]>
Co-authored-by: Marika Marszalkowski <[email protected]>
Co-authored-by: Marika Marszalkowski <[email protected]>
…s into documentation/minor-cleanup
…s into documentation/minor-cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small improvements suggested
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each model, model version, and resource group allows for a one-time deployment. | |
Deployment requires model name, model version and resource group. |
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
## 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-authored-by: Zhongpin Wang <[email protected]>
Co-authored-by: Zhongpin Wang <[email protected]>
There was a problem hiding this 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.
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[ ] (Optional) Aligned changes with the Java SDK[ ] (Optional) Release notes updated