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

support a custom HTTPX client in Client and AsyncClient #380

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

samuelcolvin
Copy link

As well as allowing a custom httpx client, this also improves type safety in Client and AsyncClient - previously self._client implicitly had type of None.

@samuelcolvin
Copy link
Author

This is needed for pydantic-ai to use this library rather than the OpenAI compatibility layer, see pydantic/pydantic-ai#242.

@mxyng
Copy link
Collaborator

mxyng commented Dec 16, 2024

Thanks for the PR. Can you provide some details in the description of why this change is necessary for pydantic-ai?

@samuelcolvin
Copy link
Author

We would like to reuse a set of http connections (encapsulated in an HTTPX client) when creating Ollama clients, so it's as cheap as possible to create new clients.

Please could you at least kick off tests.

By the way, you can set github to run tests automatically, even for new contributors; this change makes it much more attractive to contribute to the library.

@mxyng
Copy link
Collaborator

mxyng commented Dec 17, 2024

Hi can you fix the tests? ollama-python currently targets 3.8 so the union types | are not supported. Please use typing.Union

@@ -74,48 +74,54 @@
T = TypeVar('T')


class BaseClient:
class Client:
@overload
Copy link
Collaborator

Choose a reason for hiding this comment

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

These overload aren't necessary since there's no overlap between the client and non-client versions

Copy link
Author

Choose a reason for hiding this comment

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

They are to maintain proper typing, the point is you can't pass client and follow_redirects or timeout together. These overloads mean doing so will give a typing error.

ollama/_client.py Show resolved Hide resolved
ollama/_client.py Outdated Show resolved Hide resolved
@samuelcolvin
Copy link
Author

Fixed suggestions I think, by the way it would be easier for contributors and less work for you if you let tests run automatically.

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.

2 participants