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 tools #1242

Closed
wants to merge 2 commits into from
Closed

Conversation

Cruppelt
Copy link
Contributor

@Cruppelt Cruppelt commented Dec 6, 2024

Example

from ollama import Client
from pydantic import BaseModel

import instructor


class UserInfo(BaseModel):
    name: str
    age: int


client = instructor.from_ollama(Client())

user_info = client.chat.completions.create(
    model="llama3.2",
    response_model=UserInfo,
    messages=[{"role": "user", "content": "John Doe is 30 years old."}],
)

print(user_info)

Ollama hasn't currently released the application with the structured outputs or the ollama-python package so this currently needs to be run with a local build. Follow their Guide to run.

@Cruppelt Cruppelt changed the title [WIP] Ollama tools Ollama tools Dec 6, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 4ef7de6 in 1 minute and 22 seconds

More details
  • Looked at 217 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. instructor/client_ollama.py:43
  • Draft comment:
    The error message in the assertion should be more descriptive. Consider changing it to: "Mode must be one of {instructor.Mode.OLLAMA_TOOLS}".
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error message in the assertion is missing a verb, making it unclear.
2. instructor/client_ollama.py:51
  • Draft comment:
    The error message in the assertion should be more descriptive. Consider changing it to: "Client must be an instance of ollama.Client or ollama.AsyncClient".
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error message in the assertion is missing a verb, making it unclear.
3. instructor/client_ollama.py:76
  • Draft comment:
    Add a newline at the end of the file.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The file instructor/client_ollama.py is missing a newline at the end of the file, which is a common best practice.
4. pyproject.toml:44
  • Draft comment:
    The ollama-python dependency version is marked as TODO. Please specify a valid version or remove it if not yet available.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The ollama-python dependency version is marked as TODO, which is not valid for a dependency specification.
5. instructor/client_ollama.py:43
  • Draft comment:
    Assertion error messages should use f-strings to include expected values. For example:
    ), f"Mode must be one of {instructor.Mode.OLLAMA_TOOLS}"
- **Reason this comment was not posted:** 
Confidence changes required: `80%`
The assertion error messages in `instructor/client_ollama.py` are not formatted correctly. They should use f-strings to include the expected values.

</details>

<details>
<summary>6. <code>instructor/client_ollama.py:51</code></summary>

- **Draft comment:** 
Assertion error messages should use f-strings to include expected values. For example:
```suggestion
    ), f"Client must be an instance of {{ollama.Client, ollama.AsyncClient}}"
- **Reason this comment was not posted:** 
Confidence changes required: `80%`
The assertion error messages in `instructor/client_ollama.py` are not formatted correctly. They should use f-strings to include the expected values.

</details>

<details>
<summary>7. <code>instructor/client_ollama.py:25</code></summary>

- **Draft comment:** 
The default mode in the overloads of `from_ollama` should be consistent. Consider using `instructor.Mode.OLLAMA_TOOLS` for both overloads.
- **Reason this comment was not posted:** 
Confidence changes required: `80%`
The function `from_ollama` has inconsistent default mode values in its overloads, which can lead to confusion.

</details>

<details>
<summary>8. <code>pyproject.toml:44</code></summary>

- **Draft comment:** 
The version for `ollama-python` is marked as TODO. Please update it with the correct version once available.
- **Reason this comment was not posted:** 
Confidence changes required: `50%`
The `pyproject.toml` file has added a new optional dependency `ollama-python`, but it is marked as TODO. This should be updated with the correct version once available.

</details>


Workflow ID: <workflowid>`wflow_3rhNbe39GysWtmOv`</workflowid>

</details>


----
**Want Ellipsis to fix these issues?** Tag `@ellipsis-dev` in a comment. You can customize Ellipsis with :+1: / :-1: [feedback](https://docs.ellipsis.dev/review), review rules, user-specific overrides, `quiet` mode, and [more](https://docs.ellipsis.dev/config).

instructor/client_ollama.py Outdated Show resolved Hide resolved
@Cruppelt
Copy link
Contributor Author

Cruppelt commented Dec 7, 2024

Closing
image

@Cruppelt Cruppelt closed this Dec 7, 2024
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.

1 participant