Add gemma2:2b test and ollama pull format as cli #599
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
packages/cli/src/nodehost.ts
Outdated
| @@ -70,7 +70,7 @@ class ModelManager implements ModelService { | |||
| if (provider === MODEL_PROVIDER_OLLAMA) { | |||
| if (this.pulled.includes(modelid)) return { ok: true } | |||
|
|
|||
| logVerbose(`ollama: pulling ${modelid}...`) | |||
| logVerbose(`ollama pull ${model}`) | |||
There was a problem hiding this comment.
The variable model is not defined in the current scope. It seems like you meant to use modelid instead. 🤔
generated by pr-review-commit
variable_name
| return { provider: MODEL_PROVIDER_LLAMAFILE, model: "*", modelId: id } | ||
| else return { provider: MODEL_PROVIDER_OPENAI, model: id, modelId: id } | ||
| return { provider: MODEL_PROVIDER_LLAMAFILE, family: "*", model: id } | ||
| else return { provider: MODEL_PROVIDER_OPENAI, family: id, model: id } |
There was a problem hiding this comment.
The variable name change from model to family and modelId to model could potentially cause confusion and bugs in the future. It's important to ensure that variable names accurately represent the data they hold. 😊
generated by pr-review-commit
variable_name_change
| }) | ||
| if (!res.ok) { | ||
| throw new Error( | ||
| `Failed to pull model ${modelId}: ${res.status} ${res.statusText}` | ||
| `Failed to pull model ${model}: ${res.status} ${res.statusText}` |
There was a problem hiding this comment.
The variable name change from modelId to model could potentially cause confusion and bugs in the future. It's important to ensure that variable names accurately represent the data they hold. 😊
generated by pr-review-commit
variable_name_change
|
The changes are mostly about renaming the attribute 'model' to 'family' and 'modelId' to 'model' in various parts of the code. As these are just variable renames, which TypeScript would handle well, there should be no functional issues that arise from this change, unless there is code elsewhere that is not shown in this diff that relies on the old attribute names. One minor concern is the change in the log statement: - logVerbose(`ollama: pulling ${modelid}...`)
+ logVerbose(`ollama pull ${model}`)It's not functionally an issue, but the change in wording might affect the clarity of the log messages. The previous wording indicated an ongoing action ( Apart from this, LGTM 🚀.
|
| assert(!!id) | ||
| id = id.replace("-35-", "-3.5-") | ||
| const parts = id.split(":") | ||
| if (parts.length >= 3) | ||
| return { | ||
| provider: parts[0], | ||
| model: parts[1], | ||
| family: parts[1], |
There was a problem hiding this comment.
The variable model has been changed to family which might not be defined in this scope. Please ensure that family is defined and holds the correct value.
generated by pr-review-commit
variable_name
| @@ -16,23 +16,23 @@ export const OllamaCompletion: ChatCompletionHandler = async ( | |||
| return await OpenAIChatCompletion(req, cfg, options, trace) | |||
| } catch (e) { | |||
| if (isRequestError(e)) { | |||
| const { modelId } = parseModelIdentifier(req.model) | |||
| const { model } = parseModelIdentifier(req.model) | |||
There was a problem hiding this comment.
The variable modelId has been changed to model which might not be defined in this scope. Please ensure that model is defined and holds the correct value.
generated by pr-review-commit
variable_name
| @@ -70,7 +70,8 @@ class ModelManager implements ModelService { | |||
| if (provider === MODEL_PROVIDER_OLLAMA) { | |||
| if (this.pulled.includes(modelid)) return { ok: true } | |||
|
|
|||
| logVerbose(`ollama: pulling ${modelid}...`) | |||
| if (!isQuiet) | |||
| logVerbose(`ollama pull ${model}`) | |||
There was a problem hiding this comment.
The function logVerbose is not defined or imported in this file. Please make sure to define or import it before using. 🧐
generated by pr-review-commit
logVerbose_undefined
| tag: parts.slice(2).join(":"), | ||
| modelId: parts.slice(1).join(":"), | ||
| model: parts.slice(1).join(":"), |
There was a problem hiding this comment.
The tag field is not being returned when the length of parts is greater than or equal to 3. This could lead to unexpected behavior if the tag field is expected in the returned object. 😕
generated by pr-review-commit
missing_tag
| // model not installed locally | ||
| // trim v1 | ||
| const fetch = await createFetch({ trace }) | ||
| const res = await fetch(cfg.base.replace("/v1", "/api/pull"), { | ||
| method: "POST", | ||
| body: JSON.stringify({ name: modelId, stream: false }), | ||
| body: JSON.stringify({ name: model, stream: false }), |
There was a problem hiding this comment.
The API endpoint has been changed from "/v1" to "/api/pull". Please ensure that this new endpoint is correct and the server is configured to handle requests at this endpoint. 🤔
generated by pr-review-commit
api_endpoint_change
This pull request includes two commits. The first commit adds a test for gemma2:2b. The second commit updates the ollama pull format to be used as a command-line interface (CLI). These changes improve the functionality and usability of the software.
Change in Model Identifier Parsing 📚: The 'parseModelIdentifier' function in 'packages/core/src/models.ts' had its return type parameter 'model' altered to 'family'. A new return type parameter 'model' was introduced which concatenates 'family' and 'tag'.
Changes in Test Cases 👩🔬: Test cases in 'packages/core/src/models.test.ts' were updated to reflect these changes. The assert checks for 'model' were changed to 'family' and vice versa.
Change in Model Pulling 🎣: In 'packages/cli/src/nodehost.ts', a log message was edited to read 'ollama pull (model)'.
Handling Missing Models 💾: If a model is not found during a ChatCompletionHandler request in 'packages/core/src/ollama.ts', the new model identifier parsing method is employed to try and pull the model.
New Script File 🆕: A new script file (summarize-ollama-gemma2.genai.js) was added in 'packages/sample/genaisrc/'.
Remember, changes to 'packages/core/src/prompt_template.d.ts' and 'packages/core/src/prompt_type.ts' are user-facing. So, ensure to check them for any significant changes!