dspy icon indicating copy to clipboard operation
dspy copied to clipboard

New Signature class based on pydantic + unit tests

Open thomasahle opened this issue 4 months ago • 4 comments

This PR creates a new Signature class based on Pydantic, which allows for type signatures. It doesn't actually do anything with the types for now, but it's the first step.

Maybe more importantly, the PR creates 85 unit tests, covering most of dspy/, which were important to make sure the new type signature code didn't break anything, and will hopefully be useful for future refactors.

thomasahle avatar Feb 10 '24 21:02 thomasahle

I think this is good to merge (except the note above and maybe 1-2 minor things) but it's gigantic haha

okhat avatar Feb 11 '24 06:02 okhat

I tried to keep the changes outside of the tests as small as possible. But it does affect nearly eveything 😳.

Do you think there are any modules I may have forgotten to test that would also need to be updated?

thomasahle avatar Feb 11 '24 06:02 thomasahle

Hi @thomasahle Thank you for this nice PR! I have an issue using the BayesianSignatureOptimizer with extended signatures from different predictors, so I wondered if you considered this in your PR. I saw that you've "migrated to the new API" so not sure if it deals with this as well.

younes-io avatar Feb 14 '24 13:02 younes-io

I did consider it so far as I noticed it didn't work, and only tested with ChainOfThought :) Maybe I should have just created a test for Predict too, and let it be failing. But I didn't want to create a PR with failing tests... Code from test_signature_opt_bayesian: Screenshot 2024-02-14 at 4 19 54 PM

thomasahle avatar Feb 15 '24 00:02 thomasahle

Wow, this looks fantastic! I'm particularly jazzed by the unit tests (which will make refactoring much easier).

Thanks for lifting this huge load @thomasahle 🙏

@okhat what would it take from your side of things to merge this?

CyrusOfEden avatar Feb 15 '24 01:02 CyrusOfEden

@okhat, thank you for the merge; could you please add release management so it can be easy to find the latest version and the release notes (commits)?

younes-io avatar Feb 15 '24 17:02 younes-io

@younes-io This isn't merged to main yet. I merged to a branch here to have a bit more control while testing the different parts

okhat avatar Feb 15 '24 18:02 okhat

While working on the method decorators, I realized the fields property on Signature had a slight issue, as it would sometimes give output fields before input fields, which would screw up generation. I fixed it here: https://github.com/thomasahle/dspy/commit/5794984132b1862dec6e33b19234bf03e161917d

I can make a new PR to the new branch with the fix? But I can also just wait to the PR is merged into main, since it doesn't appear to break anything in the current codebase.

thomasnormal avatar Feb 15 '24 18:02 thomasnormal

@thomasahle you're now a maintainer so it's easier to do as you see fit (e.g., update the pydantic branch directly if that's easier)

okhat avatar Feb 15 '24 18:02 okhat

Sounds good. I'll still let you decide when to merge it into main :-)

There's a bunch of work I want to do once it's in main, such as actually implementing the type checking. But also setting github up to run tests automatically etc. I don't think I should start any of that work before the PR is in main though, as it will make it harder to merge the branches eventually.

thomasnormal avatar Feb 15 '24 18:02 thomasnormal

Regarding the actual type checking/fixing, we could either do it using dspy.assertions (like in my first version here https://github.com/thomasahle/dspy/blob/main/examples/types.py) or using a manual retry loop (like in my second version here https://github.com/thomasahle/dspy/blob/method/examples/functional.py#L67).

Having tried both I'm probably most fan of the second version. DSpy assertions require wrapping the Predict in Retry, while the simple checking loop could just be implemented in the forward method of Predict.

thomasnormal avatar Feb 15 '24 18:02 thomasnormal

Yeah, within-module checking is more appropriate, assertions are overpowered for this and will make composition harder here since this is a local check

okhat avatar Feb 15 '24 18:02 okhat

Honestly the signature optimizer is the piece I'm least sure of. So I'm glad you're checking it.

Also please check if the traces in test_signsture_opt.py are actually working as you would expect. Maybe there are even some more tests you can propose to stress test it further.

thomasahle avatar Feb 15 '24 21:02 thomasahle

Regarding the Field thing we discussed, the way my Signature currently works is:

class SignatureMeta(type(BaseModel)):
    ...
class Signature(BaseModel, metaclass=SignatureMeta):
    pass

The goal was to be able to do things like

class MySignature(Signature):
    ...

as in the current code, where the subclass MySignature becomes the object you work with, rather than typical Pydantic use cases, where instances of MySignature would be the main objects to interact with.

Anyway, as you've seen MySignature can be used exactly like a pydantic type signature. And Input/OutputField are just defined as

def InputField(**kwargs):
    return pydantic.Field(**kwargs,  __dspy_field_type="input")
def OutputField(**kwargs):
    return pydantic.Field(**kwargs, __dspy_field_type="output")

However, this mean that if a user of DSPy writes

class MySignature(Signature):
    positive: int = InputField(gt=0)

The keyword gt=0 could potentially collide with a keyword we'd like to add to DSPy.

In general pydantic classes are actually a quite rich language of validators and detailed specifications for types. This is how it improves over something more simple like dataclasses in python.

The simple solution would be prefixing all dspy arguments:

class MySignature(Signature):
    positive: str = InputField(dspy_desc="abc", description="abc")

But it's not that pretty.

thomasahle avatar Feb 16 '24 17:02 thomasahle

Maybe we should prefix the other way around — special keyword for pydantic-y things.

okhat avatar Feb 18 '24 22:02 okhat

Anyway : any choices that are backward compatible are fine with me in the short term until we can make a major release

okhat avatar Feb 18 '24 22:02 okhat

re: description, we should move towards pulling from the pydantic description.

For the other ones, extra keywords is fine. I imagine we'll have a bigger breaking API change with 3.0 so whatever we decide here isn't gonna live forever and is reversible

CyrusOfEden avatar Feb 19 '24 17:02 CyrusOfEden

Maybe we should prefix the other way around — special keyword for pydantic-y things.

Let's look at the parameters that are specific and shared between the two:

pydantic only:

  • gt/lt: for integer sizes
  • pattern: regular expression the output must match
  • min_length/max_length: number of characters

dspy only:

  • prefix: When you want to the Name) to be different from the variable name
  • format: When you want to override the default data type serializtion

shared:

  • desc/description: Human-readable description

It seems to me the pydantic field arguments are things we "want" users to define, while the dspy-only arguments are things that the user really shouldn't have to care about. Stuff that should "just work" and only be changed by optimizers etc.

(Actually we could even use the pydantic title or alias as an equivalent of dspy prefix. So that's even less stuff left to consolidate.)

Anyway : any choices that are backward compatible are fine with me in the short term until we can make a major release

Does that mean you are ok with field.json_schema_extra['prefix'] or field.dspy['prefix'] or something for now?

thomasahle avatar Feb 19 '24 23:02 thomasahle