Skip to content
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

test: convert Google Gemini tests to VCR #118

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

codefromthecrypt
Copy link
Collaborator

@codefromthecrypt
Copy link
Collaborator Author

@baxen I'll move each pending PR from exchange to here one-by-one, here's the first!

@@ -128,4 +149,4 @@ def vision(provider_cls: Type[Provider], model: str, **kwargs) -> Tuple[Message,
content=[ToolResult(tool_use_id="xyz", output='"image:tests/test_image.png"')],
),
]
return provider.complete(model=model, system=system, messages=messages, tools=None, **kwargs)
return provider.complete(model=model, system=system, messages=messages, tools=(), **kwargs)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of IDE warnings about call sites we use, as it is at the moment tools: Tuple[Tool]. So, this clears warnings about passing None. Though I wonder if it shouldn't be tools: Tuple[Tool, ...]? Seems that would be more explicit about possibly none, and also clear some other warnings. If you feel this is worthwhile I can in a follow-up or different PR.

Screenshot 2024-10-05 at 7 32 24 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lamchau
Copy link
Collaborator

lamchau commented Oct 5, 2024

:nit: could we format the json so it's easier to read?

        string: |-
          {
            "candidates": [
              {
                "content": { ... }
               }

@codefromthecrypt
Copy link
Collaborator Author

@lamchau

:nit: could we format the json so it's easier to read?

technically yes, it will require some engineering to write a custom serializer as vcr has no options built-in. I'm not opposed to this, but it also will affect the openai data already here, and could be done as a separate task. Is it ok if we decouple this? You can assign that issue to me which I can try on a weekend or a flight.

@lamchau
Copy link
Collaborator

lamchau commented Oct 8, 2024

@codefromthecrypt sure! just a nit - i just like to know what's being passed in

just for learning: is the custom serializer required to dump the request/response to yaml? never used it before so i was naively thinking using a yq transform for post-processing - the serializer sounds much more robust (and involved)

edit: made issue #127, can't seem to assign it - thanks for taking a look!

Copy link
Collaborator

@baxen baxen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@baxen baxen merged commit 3eb5a93 into block:main Oct 10, 2024
4 checks passed
ahau-square added a commit that referenced this pull request Oct 10, 2024
* main:
  feat: add groq provider (#134)
  feat: add a deep thinking reasoner model (o1-preview/mini) (#68)
  fix: use concrete SessionNotifier (#135)
  feat: add guards to session management (#101)
  fix: Set default model configuration for the Google provider. (#131)
  test: convert Google Gemini tests to VCR (#118)
  chore: Add goose providers list command (#116)
  docs: working ollama for desktop (#125)
  docs: format and clean up warnings/errors (#120)
  docs: update deploy workflow (#124)
  feat: Implement a goose run command (#121)
@codefromthecrypt codefromthecrypt deleted the gemini-tests branch October 10, 2024 23:59
@codefromthecrypt
Copy link
Collaborator Author

@lamchau

just for learning: is the custom serializer required to dump the request/response to yaml? never used it before so i was naively thinking using a yq transform for post-processing - the serializer sounds much more robust (and involved)

So, I didn't consider yq (like something we could do in just format), it is possible that doing that wouldn't taint the input expectations vcrpy and look like request drift. That could be the easier way out vs a custom serializer, which is the current suggestion from the project. kevin1024/vcrpy#580

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants