feat(llm-batch): add LlmBatchModel type for batch-supported models - #2177
Open
InjunPark-sap wants to merge 5 commits into
Open
feat(llm-batch): add LlmBatchModel type for batch-supported models#2177InjunPark-sap wants to merge 5 commits into
InjunPark-sap wants to merge 5 commits into
Conversation
Contributor
|
👋 Hi — I'm PR Bot, your SAP code review assistant. I'll automatically review your pull requests for code quality, security, and SAP compliance. Get an overview of what I do → What I do
Key commands
Configure me for your teamCreate {
"$schema": "https://devops-insights-pr-bot.cfapps.eu10-004.hana.ondemand.com/schema/pull_request_bot.json",
"features": {
"control_panel": false,
"summarize": {
"auto_generate_summary": true,
"auto_insert_summary": true,
"auto_run_on_draft_pr": true,
"use_custom_summarize_prompt": false,
"use_custom_summarize_output_template": false,
"excluded_paths": [],
"auto_exclude_authors": []
},
"review": {
"auto_generate_review": true,
"auto_run_on_draft_pr": false,
"use_custom_review_focus": false,
"excluded_paths": [],
"auto_exclude_authors": []
},
"sonar_fix": {
"enable": true,
"excluded_rules": []
},
"pipeline_fix": {
"enable": true
}
},
"excluded_paths": []
}
|
InjunPark-sap
marked this pull request as ready for review
August 20, 2026 08:43
Contributor
SummaryThe following content is AI-generated and provides a summary of the pull request: New Feature: Add
|
davidkna-sap
requested changes
Aug 20, 2026
Comment on lines
+46
to
+47
| "make-llm-batch-experimental": "node ../../scripts/postgenerate-llm-batch.ts ./src/client/batch-service/batches-api.ts", | ||
| "patch-batch-model-type": "node ../../scripts/postgenerate-llm-batch.ts ./src/client/batch-service/schema/batch-create-request.ts", |
Member
There was a problem hiding this comment.
[pp] Please handle these as a single invocation of postgenerate-llm-batch.ts
Comment on lines
+84
to
+87
| // Header/title rows vary, so filter by model-name shape instead of a fixed | ||
| // slice offset: keep only cells that look like a model id (lowercase, | ||
| // digits, dots, dashes). This rejects header labels like "Model"/"Region". | ||
| const isModelName = s => /^[a-z0-9][a-z0-9.-]*$/.test(s); |
Member
There was a problem hiding this comment.
[q] Other row extractions do not have this, is this necessary?
Suggested change
| // Header/title rows vary, so filter by model-name shape instead of a fixed | |
| // slice offset: keep only cells that look like a model id (lowercase, | |
| // digits, dots, dashes). This rejects header labels like "Model"/"Region". | |
| const isModelName = s => /^[a-z0-9][a-z0-9.-]*$/.test(s); |
| // Identified by a "region" column with no "orchestration" and no | ||
| // "suggested replacement" column (distinguishes it from active/retired tables). | ||
|
|
||
| function findBatchTable() { |
Member
There was a problem hiding this comment.
[pp] I assume this works but it might be easier to look for the first table past a <strong>/<b>/<hN> Batch Consumption string.
Comment on lines
+348
to
+349
| batchCurrent.size !== batchNew.size || | ||
| [...batchNew].some(m => !batchCurrent.has(m)); |
Member
There was a problem hiding this comment.
[pp]
Suggested change
| batchCurrent.size !== batchNew.size || | |
| [...batchNew].some(m => !batchCurrent.has(m)); | |
| batchCurrent.symmetricDifference(batchNew).size > 0; |
|
|
||
| const SAP_MODELS_PATH = resolve(import.meta.dirname, 'sap-models.json'); | ||
|
|
||
| const SAP_BATCH_MODELS_PATH = resolve(import.meta.dirname, 'sap-batch-models.json'); |
Member
There was a problem hiding this comment.
[pp] I would prefer adding the new information to SAP_MODELS_PATH as part of an extra key in the main file.
Comment on lines
+465
to
+467
| if (batchChanged) { | ||
| bumps.push("'@sap-ai-sdk/llm-batch': minor"); | ||
| } |
Member
There was a problem hiding this comment.
As we don't bump extra packages when bumping other model types, I would also avoid doing that here.
| batchCurrent.size !== batchNew.size || | ||
| [...batchNew].some(m => !batchCurrent.has(m)); | ||
|
|
||
| const allAdded = Object.entries(typeToActiveModels).flatMap(([typeName, activeModels]) => { |
Member
There was a problem hiding this comment.
[req] Please add changeset lines for changes (additions/removals) to the batch model list.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Closes SAP/ai-sdk-js-backlog#609.
What this PR does and why it is needed
Adds a
LlmBatchModeltype soBatchCreateRequest.spec.modelis typed instead of plainstring, giving IDE autocomplete for batch-supported models while staying backward compatible (any string is still accepted viaLiteralUnion).LlmBatchModelin@sap-ai-sdk/core, re-exported from@sap-ai-sdk/llm-batch.BatchCreateRequest.spec.modeltoLlmBatchModelvia the postgenerate step (generated file is never hand-edited; anchor miss hard-fails).scripts/sap-batch-models.jsonas the ground-truth batch model list (6 models from the LLM batch service config; the SAP Notes list is intentionally incomplete).update-modelsskill + scraper to sync the SAP Notes batch section non-destructively (never shrinks the ground-truth file; auto-bumps thellm-batchchangeset).Definition of Done