updates token handling for Azure #234
Conversation
|
Thank you for taking the time to contribute! Some comments:
|
|
Tony and I talked about the PR a few days ago (we work together). This does better handling of the Azure token. Before we were generating a token way too often. |
| }, | ||
| error = function(e) NULL | ||
| ) | ||
| token <- retrieve_azure_token_object() %>% suppressMessages() |
There was a problem hiding this comment.
we no longer have magrittr dependency so we are using base pipe
| @@ -10,6 +10,9 @@ | |||
| )) | |||
| } | |||
|
|
|||
| require("Microsoft365R") | |||
There was a problem hiding this comment.
Would be good to refactor this so that this is an optional dependency. Most packages won't need it since most don't use Azure OpenAI.
| #' a function that determines the appropriate directory to cache a token | ||
| #' @export | ||
| gptstudio_cache_directory = function(){ | ||
| rappdirs::user_data_dir(appname = glue::glue("gptstudio"), |
There was a problem hiding this comment.
can we switch this to tools::R_user_dir()?
| @@ -110,31 +110,33 @@ query_api_azure_openai <- | |||
| retrieve_azure_token <- function() { | |||
| rlang::check_installed("AzureRMR") | |||
There was a problem hiding this comment.
swap out for Microsoft365R?
|
|
||
| invisible(token$token$credentials$access_token) | ||
| client <- Microsoft365R:::do_login(tenant = Sys.getenv("AZURE_OPENAI_TENANT_ID"), |
There was a problem hiding this comment.
CRAN might yell about us depending on a hidden function, but I'm not sure what the rules are.
There was a problem hiding this comment.
On these situations I would prefer to reverse-engineer the function to have a local version instead of a dependency. But the level of effort required varies between cases
Thanks @JamesHWade , I'll move away from this one then |
Description of changes
This pull request addresses token management for Azure Open AI tokens. The token is saved to a gptstudio specific directory and ensures that tokens are properly refreshed as needed.