-
Notifications
You must be signed in to change notification settings - Fork 3
fix: Prepare for upcoming ellmer v0.4.0 release #114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Prepare for upcoming ellmer v0.4.0 release #114
Conversation
kalimu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maciej I will forward you also an email from CRAN. Please check it out.
|
Thanks @gadenbuie I will merge your changes into our feature branch, fix other issues with private methods and make any updates for |
3580403
into
r-world-devs:maciekbanas/115/ellmer-fixes-for-cran
| my_project <- my_project |> | ||
| set_llm(provider = "openai", echo = "all") | ||
| expect_equal( | ||
| # Please don't reach into the private namespace of external R6 classes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand there is now no public way to check echo parameter - in that case you suggest other way to test setting echo to all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I wouldn't recommend testing this. If you're only passing echo from set_llm() to chat_*(), I think there's very little utility in having a test, unless some part of your package explicitly depends on the values of echo used by ellmer.
My view is that tests in your package shouldn't cover behavior from another package. What would happen if ellmer changed the allowed values of echo? If some part of your package would break as a result, you should write a test around that potential breakage. But if users of your package would be able to pass those new values through echo, then you don't need a test.
The fact that this test just covers "was echo set?" seems to me to be a sign that the test isn't necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right @gadenbuie .
|
@gadenbuie when do you plan the |
It's already on CRAN as of Nov. 15! 😄 |
|
All right, that is probably reason for failing checks on CRAN. Thanks! |
Hi there! We're preparing for the next ellmer release (v0.4.0) and reverse dependency checks turned up a couple of issues with the new release in GitAI.
The biggest change I was able to find and fix is the move from using an
api_keyargument in favor of the newcredentialsargument. (Discussed in tidyverse/ellmer#613, implemented in tidyverse/ellmer#799.)credentialsis by default a zero-argument function that returns the API key as a string.I updated
chat_openai_mocked()to takeapi_keyas currently but to wrap it into the kind of function expected bycredentials.If your tests uncover other impacts from the new ellmer v0.4.0 release candidate, I'll be happy to help you update GitAI as needed.
Relatedly, while in this same area, I noticed that in a few places you're reacing into the private methods of the
Chatclass, or using internal functions from the ellmer package.I fixed two of these instances, but I strongly urge you to take a pass through your tests and to avoid reaching into ellmer's private namespace or private methods in ellmer's classes.
Typically, there are public methods that you can use for your testing, but we also welcome feedback in https://github.com/tidyverse/ellmer and would happily field requests to make useful methods public.
When you write tests around private methods and functions, you risk your package tests failing when we make updates, which in turn can delay updates and new releases and can increase the risk that your package might be removed from CRAN due to internal changes in another package.