DAGWorks-Inc / burr

Build applications that make decisions (chatbots, agents, simulations, etc...). Monitor, trace, persist, and execute on your own infrastructure.
https://burr.dagworks.io
BSD 3-Clause Clear License
1.19k stars 67 forks source link

pydantic: uninformative error #385

Open zilto opened 2 weeks ago

zilto commented 2 weeks ago

I'm building an application with burr.integrations.pydantic and building the graph + application fails. The following error message isn't helpful because it exposes internal implementation (i.e., the key "state" is nowhere in my code.

Traceback (most recent call last):
  File "/home/tjean/projects/form_helper/app.py", line 45, in <module>
    @pydantic_action(reads=["form"], writes=["current_field"])
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tjean/projects/form_helper/.venv/lib/python3.11/site-packages/burr/integrations/pydantic.py", line 180, in decorator
    itype, otype = _validate_and_extract_signature_types(fn)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/tjean/projects/form_helper/.venv/lib/python3.11/site-packages/burr/integrations/pydantic.py", line 116, in _validate_and_extract_signature_types
    if (state_model := type_hints["state"]) is inspect.Parameter.empty or not issubclass(
                       ~~~~~~~~~~^^^^^^^^^
KeyError: 'state'

The only hint is to look at "line 45", but I can't tell what's the issue. I ended up finding that the issue is the function signature on line 46.

Original:

@pydantic_action(reads=["form"], writes=["current_field"])
def prompt_user(state):

Fix:

@pydantic_action(reads=["form"], writes=["current_field"])
def prompt_user(state: BurrState):
elijahbenizzy commented 2 weeks ago

This should be as simple as throwing the appropriate error message here

rajatkriplani commented 3 days ago

Hi @elijahbenizzy can't I just simply add:

state_model = type_hints.get("state", None)

in line 115 of pydantic.py and then change the if condition to raise ValueError

elijahbenizzy commented 3 days ago

here

Yep, I think that will work. In fact, you don't have to have the None there, you can just do .get(...) which returns None if it's not there!

rajatkriplani commented 3 days ago

Yep, I think that will work. In fact, you don't have to have the None there, you can just do .get(...) which returns None if it's not there!

Oh yes, thanks for reminding me that. Please let me know if if statement is checking all the conditions correctly:

state_model = type_hints.get("state")

    if state_model is None or state_model is inspect.Parameter.empty or not issubclass(state_model, pydantic.BaseModel):
        raise ValueError(
            f"Function fn: {fn.__qualname__} is not a valid pydantic action. "
            "The 'state' parameter must be annotated with a type extending pydantic.BaseModel."
        )
rajatkriplani commented 1 day ago

Hey @elijahbenizzy @zilto can anyone of you give me feedback on the if condition?

elijahbenizzy commented 1 day ago

Hey @elijahbenizzy @zilto can anyone of you give me feedback on the if condition?

Hey! Looks good at first glance. Open a PR and add some tests, then I can give you more feedback there!