Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the installation logic by introducing a new resolveInstallAction function to centralize the decision-making process for different backends and environments. This is a positive change that improves code clarity and testability, as demonstrated by the new unit tests. However, I've identified a high-severity issue regarding code structure where state is mutated after it has been used, which could lead to confusion and bugs in the future. My review includes a suggestion to address this for improved maintainability.
| // For Desktop+WSL with vllm, override engine kind to Moby. | ||
| vllmOnWSL := false | ||
| if isWSL && opts.backend == vllm.Name { | ||
| engineKind = types.ModelRunnerEngineKindMoby | ||
| vllmOnWSL = true | ||
| } |
There was a problem hiding this comment.
This logic to override engineKind for vLLM on WSL is placed after resolveInstallAction has been called. This is confusing because resolveInstallAction was called with the original engineKind, and now the state is being mutated. This can make the code harder to reason about and maintain.
To improve clarity and centralize the installation strategy, this logic should be moved to before the call to resolveInstallAction (around line 308). This ensures that resolveInstallAction operates on the effective engineKind for the installation.
1 participant
No description provided.