dspy icon indicating copy to clipboard operation
dspy copied to clipboard

Support convenience import for `OllamaLocal`

Open forwardprogress opened this issue 1 year ago • 7 comments

Summary

tldr; These changes add a convenience import for the OllamaLocal import introduced in https://github.com/stanfordnlp/dspy/pull/262 consistent with what's done for GPT-3.5, et al. I also added a minimal test to show the before/after comparison.

I began working through intro.ipynb today and wanted to use a model I was hosting locally with Ollama instead of GPT-3.5. Though the OpenAI symbol is actually a reference to dsp.modules.GPT3, there's some hoisting to make it a little more convenient to access:

turbo = dspy.OpenAI(model='gpt-3.5-turbo')

I expected to do the same for Ollama:

nous_hermes2_mixtral = dsp.OllamaLocal(model="nous-hermes2-mixtral", model_type="chat")

but found it wasn't supported. I've made the appropriate change to dsp/modules/__init__.py to make the Ollama module consistent with its siblings.

I wanted to add a test demonstrating the change, but couldn't find any unit tests. I've added a new tests/ package at the root of the repo to host all test suites, which I understand to be common convention. I've avoided adding any dependencies by relying on the built-in unit testing library in Python.

⚠️ These changes also propose running the new unit test as part of GitHub CI checks. I'm unfamiliar with GitHub's CI system, so if the CI change is acceptable, it deserves some scrutiny to make sure I didn't make a silly mistake.

Testing

I added a unit test that verifies the convenience import is now supported. Unit tests can be executed from the command line with this command:

% python -m unittest discover tests/

Before:

E
======================================================================
ERROR: test_convenience_import_of_dsp_ollama_client (test_dsp.DspModuleImportsTest.test_convenience_import_of_dsp_ollama_client)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/forwardprogress/Developer/stanfordnlp/dspy/tests/test_dsp.py", line 6, in test_convenience_import_of_dsp_ollama_client
    from dsp import OllamaLocal
ImportError: cannot import name 'OllamaLocal' from 'dsp' (/Users/forwardprogress/Developer/stanfordnlp/dspy/dsp/__init__.py)

----------------------------------------------------------------------
Ran 1 test in 0.479s

FAILED (errors=1)

After:

.
----------------------------------------------------------------------
Ran 1 test in 0.484s

OK

forwardprogress avatar Jan 22 '24 07:01 forwardprogress

Hey thanks so much, but this may create a new dependency for everyone who doesn't need it?

okhat avatar Jan 22 '24 19:01 okhat

Hey thanks so much, but this may create a new dependency for everyone who doesn't need it?

Ah, that's a great point that I didn't fully consider. I think the Ollama module requires no additional dependencies but the HF change does.

If I updated the changes to only hoist OllamaLocal, would that be agreeable?

To be clear, I'm operating with the presumption that it's desirable for most (or all?) modules to have these convenience imports. If that's not the case, I'm happy to get that feedback.

forwardprogress avatar Jan 22 '24 19:01 forwardprogress

Whelp, this PR got whittled down to a lot less than I expected. 😉

forwardprogress avatar Jan 23 '24 04:01 forwardprogress

Alright @okhat @piotrlaczkowski I don't think I'm getting traction with this change so I'll plan to close it out sometime tomorrow if I don't hear back.

forwardprogress avatar Jan 25 '24 03:01 forwardprogress

No reason to not merge this, it's a bugfix IMO!

psykhi avatar Jan 25 '24 09:01 psykhi

I’m confused. This creates a dependency on other tools that everyone now will need to install?

okhat avatar Jan 25 '24 09:01 okhat

From ollama.py

from dsp.modules.lm import LM
from typing import Any, Literal, Optional

import os, multiprocessing, datetime, hashlib
import requests

import json

Aren't those already there/ in the standard lib? Sorry if I'm missing the obvious 😅

psykhi avatar Jan 25 '24 15:01 psykhi

This creates a dependency on other tools that everyone now will need to install?

Nope!

I think there may be some confusion that this adds a dependency on Ollama's brand-new ollama package, but this isn't that. The OllamaLocal client I'm referring to in this PR's discussion is already implemented in dspy without dependencies on other packages.

https://github.com/stanfordnlp/dspy/blob/967bf9670a6bf5730f8c71378604847253940e26/dsp/modules/ollama.py#L23-L25

This change just makes it as easy to access the client as easy as it is to access the OpenAI client.

turbo = dspy.OpenAI(model='gpt-3.5-turbo')

The change makes access to LM clients a little more consistent, but it's easy to access the OllamaLocal client by writing this instead:

from dsp.modules.ollama import OllamaLocal

That being said, this a low-value change and it's not worth distracting @okhat with such a mundane change. Maybe later on, I'll look into what it would look to adopt the new official Ollama client.

Cheers!

forwardprogress avatar Jan 28 '24 15:01 forwardprogress