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

ollama: use official client #1036

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

Conversation

treywelsh
Copy link
Contributor

@treywelsh treywelsh commented Sep 24, 2024

Close #1035

Conflict #1022
Need to see if it fix #961

PR Checklist

  • Read the Contributing documentation.
  • Read the Code of conduct documentation.
  • Name your Pull Request title clearly, concisely, and prefixed with the name of the primarily affected package you changed according to Good commit messages (such as memory: add interfaces for X, Y or util: add whizzbang helpers).
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. Fixes #123).
  • Describes the source of new concepts.
  • References existing implementations as appropriate.
  • Contains test coverage for new functions.
  • Passes all golangci-lint checks.

@treywelsh treywelsh marked this pull request as draft September 24, 2024 15:23
@treywelsh treywelsh force-pushed the use_ollama_client branch 2 times, most recently from de38692 to c61a01c Compare September 24, 2024 15:35
@treywelsh treywelsh mentioned this pull request Sep 24, 2024
9 tasks
@treywelsh treywelsh marked this pull request as ready for review September 30, 2024 08:32
@pdxrlj
Copy link

pdxrlj commented Oct 9, 2024

I look forward to it, and I can use it together, so when can I use it?

@treywelsh
Copy link
Contributor Author

treywelsh commented Oct 10, 2024

There's a problem with this PR with null option values (0, "", etc.).
The llms.CallOptions struct doesn't allow to differentiate between when an option is set to 0 or not set (for numeric types int, float64...).
The official ollama client take options via the input type map[string]any, so when building the map (via makeOllamaOptionsFromOptions) I finally have a temperature, penalties etc. with a value of 0 even if the user didn't set the option.
The langchaingo "internal" clients seems to Marshal a struct with some omitempty markers at some point (which remove null values from the payload) or copy the null value.

For now I see two ways to fix this in refactoring the generic llms package:

  1. either modifying llms.CallOptions fields as pointers (so nil value when not set), but this force to modify the other clients option management for non nil. But the fields are exported and this may break something
  2. or like in chains: fix issue with overriding defaults in chainCallOption #632 in adding a boolean per field. Infortunately the boolean fields needs to be exported to be usable in the ollama package but this breaks nothing

To move forward and quickly propose a solution I go for 2. but I'm open to discuss and modify this as I'm not fully confident with this change.

@pdxrlj you can try this PR code if you want, feel free to give feedback on it

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.

ollama: internal client is no more up to date
2 participants