remove vscode azure session #692
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const { provider } = parseModelIdentifier(modelId) | ||
| if (provider === MODEL_PROVIDER_AZURE) | ||
| throw new Error("Azure end point not configured") | ||
| } |
There was a problem hiding this comment.
The error "Azure end point not configured" is thrown but not caught. This could lead to unexpected application termination. Consider handling this error to improve the application's robustness. 🛠️
generated by pr-review-commit
unhandled_error
packages/core/src/connection.ts
Outdated
| await writeText(DOT_ENV_FILENAME, src) | ||
| // const { config } = dotEnvTemplate(provider, apiType) | ||
| // const current = await tryReadText(DOT_ENV_FILENAME) | ||
| //await writeText(DOT_ENV_FILENAME, config) |
There was a problem hiding this comment.
There is a block of code that has been commented out. If this code is not needed, it is better to remove it to keep the codebase clean and maintainable. 🧹
generated by pr-review-commit
commented_code
packages/vscode/src/lmaccess.ts
Outdated
| res.provider, | ||
| res.apiType | ||
| vscode.window.showWarningMessage( | ||
| `${TOOL_NAME} - model connection not configured.` | ||
| ) |
There was a problem hiding this comment.
The function vscode.window.showWarningMessage is called without error handling. If the function fails, the error will not be caught, which could lead to unexpected behavior. Consider adding error handling. 🚀
generated by pr-review-commit
missing_error_handling
|
The changes mainly focus on the removal of Azure authentication and token management logic.
Concerns:
Since this pull request removes a substantial amount of code related to Azure tokens and authentication, I would recommend confirming whether these changes won't affect any existing functionalities or if there is an alternative token management system in place. Without further context on why these changes are being made, it's difficult to provide a definitively positive or negative review. If all dependencies on Azure were indeed removed or replaced as intended, then, LGTM 🚀. However, if there are still functionalities dependent on Azure tokens, then these changes might cause issues.
|
… process in the VSCode extension
| @@ -214,7 +214,7 @@ for GitHub Models, you can use the `GITHUB_MODELS_TOKEN` variable instead. | |||
| ## Azure OpenAI <a id="azure" href=""></a> | |||
|
|
|||
| The [Azure OpenAI](https://learn.microsoft.com/en-us/azure/ai-services/openai/reference#chat-completions) provider, `azure` uses the `AZURE_OPENAI_...` environment variables. | |||
There was a problem hiding this comment.
Duplicate content found. The sentence "You can use a managed identity (recommended) or a API key to authenticate with the Azure OpenAI service." is repeated.
generated by pr-docs-review-commit
duplicate_content
|
|
||
| ```sh | ||
| az login | ||
| ``` |
There was a problem hiding this comment.
Content has been relocated. Instructions for logging in with Azure CLI have been moved from a subsection into a list item.
generated by pr-docs-review-commit
content_relocation
| </li> | ||
|
|
||
| <li> | ||
|
|
||
| Update the `model` field in the `script` function to match the model deployment name in your Azure resource. |
There was a problem hiding this comment.
Incorrect reference in the documentation. The model field should be updated to match the model deployment name, but the provided code snippet incorrectly references 'model: "azure:deployment-id"' which is not a valid deployment name.
generated by pr-docs-review-commit
incorrect_reference
| @@ -214,7 +214,7 @@ for GitHub Models, you can use the `GITHUB_MODELS_TOKEN` variable instead. | |||
| ## Azure OpenAI <a id="azure" href=""></a> | |||
|
|
|||
| The [Azure OpenAI](https://learn.microsoft.com/en-us/azure/ai-services/openai/reference#chat-completions) provider, `azure` uses the `AZURE_OPENAI_...` environment variables. | |||
| You can use a managed identity (recommended) or a API key to authenticate with the Azure OpenAI service. | |||
| You can use a managed identity (recommended) or a API key to authenticate with the Azure OpenAI service. | |||
| You can also use a service principal as documented in [automation](/genaiscript/getting-started/automating-scripts). | |||
There was a problem hiding this comment.
The link provided in "automation" might be broken or incorrect. It should be verified if it correctly leads to the automation documentation.
generated by pr-docs-review-commit
broken_link
| const { provider } = parseModelIdentifier(modelId) | ||
| if (provider === MODEL_PROVIDER_AZURE) | ||
| throw new Error("Azure end point not configured") | ||
| } |
There was a problem hiding this comment.
The tok object is not checked for null or undefined before accessing its token property. This could lead to a runtime error if tok is null or undefined. Consider adding a null check before accessing the token property. 🛠️
generated by pr-review-commit
missing_check
| vscode.commands.executeCommand( | ||
| "genaiscript.connection.configure" | ||
| ) | ||
| }) |
There was a problem hiding this comment.
The then function is used after showWarningMessage, but the function inside then is not awaited. This could lead to unexpected behavior as the function might not complete before the next line of code is executed. Consider using await with then or converting the promise chain to async/await for better readability and error handling. 🔄
generated by pr-review-commit
async_await
| @@ -198,20 +187,6 @@ export class VSCodeHost extends EventTarget implements Host { | |||
| modelId, | |||
| options | |||
| ) | |||
There was a problem hiding this comment.
The askToken variable is declared but never used. This could lead to confusion for other developers reading the code. Consider removing unused variables to improve code readability. 🧹
generated by pr-review-commit
unused_code
it's all cli now
📝 The 'NodeHost' class has been updated with an error check for Azure endpoints. This check helps validate whether or not the Azure token is configured, thereby ensuring that the 'provider' property of the model has been correctly set as the Azure model provider. 🛠️
🗑️ The process to update '.env' file in the 'updateConnectionConfiguration' function of 'Connection.ts' file has been commented out. Previously, this process involved fetching the relevant configuration from the dotenv template, and then updating the '.env' file with this configuration. 🔄
❗ The 'AzureManager' class along with its associated file 'azuremanager.ts' has been removed. This class was initially responsible for handling logic associated with Azure sessions, like invalidating sessions and fetching new ones when the former were not available. This change might be indicative of a shift in how Azure sessions are to be managed moving forward.
💬 The 'pickLanguageModel' function in 'lmaccess.ts' has been modified to display a warning message if the model connection is not configured. Before, a 'genaiscript.connection.configure' command would be executed in this scenario.
❌ Moreover, several functionalities from the 'VSCodeHost' class have been removed. This includes the removal of methods related to Azure chats, including asking for an Azure token and creating new Azure chats.
💡 These changes suggest an overall intent of the modification to be around redefining how Azure support is incorporated into the codebase, with several Azure-related functionalities being commented out, removed, or flagged for errors.‼️