Feat: Add Optional LLM Helper#20
Conversation
DeepanshKhurana
requested review from
jakubnowicki
and removed request for
TymekDev
vituri
left a comment
vituri
left a comment
There was a problem hiding this comment.
This PR is really nice!! Why read all the logs if we can make a robot read it and even give the solution? It will save the user several Google searches. As you know, we think this is so useful that we already want to embed in another app.
I wrote some remarks, but the majority does not require any action: they are just me thinking out loud or making questions to understand your reasoning. You can mark them as solved. If you make some crucial changes, ping me again to review.
PS: I see that you also fixed lots of linter errors and styling; I did not focus on these changes since the CI is green.
| #' @return A character vector of valid LLM providers | ||
| get_valid_providers <- function( | ||
| ) { | ||
| ellmer_functions <- ls(ellmer)[ |
There was a problem hiding this comment.
The ls function has an argument called pattern, which can be used as follows:
ls(ellmer, pattern = "^chat_")
Do you think this could cut the grepl part here?
| get_llm_function <- function( | ||
| llm_config = get_llm_config() | ||
| ) { | ||
| ellmer[[glue("chat_{llm_config$provider}")]] |
There was a problem hiding this comment.
Ah, now I get your observation after the "nolint" in the beginning.
The user passes a provider and you have to check if the provider exists among many "chat_***" functions; and finally, here you have to return this function from the ellmer list.
| config[ | ||
| get | ||
| ], | ||
| ellmer, #nolint: we do use this package just in a separate notation [[ ]] |
There was a problem hiding this comment.
Can we be specific as to which linter is being ignored? Something like
ellmer, # nolint: box_unused_att_pkg_linter
| api_key = llm_config$api_key, | ||
| model = llm_config$model, | ||
| system_prompt = llm_config$system_prompt, | ||
| seed = 42, |
There was a problem hiding this comment.
Setting a seed will guarantee that the results are always the same, giving the same input?
| system_prompt = llm_config$system_prompt, | ||
| seed = 42, | ||
| api_args = list( | ||
| temperature = 0 |
There was a problem hiding this comment.
Genuine question: why set the temperature to zero? Is this too restrictive? I never played with this parameter. What if the user find the answer unhelpful and want to try the prompt again and again until something useful appears?
| ) | ||
| }) | ||
|
|
||
| shiny$observeEvent(llm_result(), { |
There was a problem hiding this comment.
Just nitpicking: I often prefer observe({...}) |> bindEvent(...) instead of observeEvent because the former can be cached more easily. Looking into the docs, it seems to be just a matter of taste for now (maybe they will deprecate it in the future?)
bindEvent() was added in Shiny 1.6.0. When it is used with reactive() and observe(), it does the same thing as eventReactive() and observeEvent(). However, bindEvent() is more flexible: it can be combined with bindCache(), and it can also be used with render functions (like renderText() and renderPlot()).
| processed_logs <- shiny$reactive({ | ||
| logs_data() %>% | ||
| pmap_dfr( | ||
| ~ { |
There was a problem hiding this comment.
Another nitpick (sorry!!): using formula is regarded as "old style" according to purrr docs:
A formula, e.g. ~ .x + 1. You must use .x to refer to the first argument. Only recommended if you require backward compatibility with older versions of R.
If the required R version is > 4.1, we can even start using the new pipe |> (supposing, of course, that we don't use anything fancy from the old pipe operator).
2 participants
Closes #19
Description
config.ymlblock for LLMs. Only params present in all functions of thechat_*family of functions fromellmerare used for now. These are also more colloquial and common parameters.modalDialog. There are other ways that are a little less brittle but I found this to work with exceptional consistency since the classes are specified in the system prompt.box.lintersproperly + carries over some of those lint fixes.Demo
Screen.Recording.2025-04-13.at.8.38.46.PM.mov
Definition of Done
R CMD check, linter, unit tests, spelling)..Rdfiles withroxygen2::roxygenise())