Skip to content

Conversation

@pmeier
Copy link
Member

@pmeier pmeier commented Jun 28, 2024

This PR changes two things:

  1. The previous implementation for the extraction of the protocol models relied on the fact that the concrete subclasses use the exact same names for the protocol parameters, i.e. the parameters defined by the protocol base class. This is rather restricting, undocumented, and has bitten us before (Embedding pr #369 (comment)). With this PR we identify the extra parameters by skipping the first n parameters in the concrete signature, where n is the number of protocol parameters.
  2. Previously we just passed through the Pydantic validation error in case of a failure. While this contains all the information you need if you know the internals of Ragna, it is hard to parse for users. Especially, because they don't have any touching point with Pydantic and we are thus "leaking internals". This PR adds logic to parse the raw errors and format them in a user friendly way.

For example:

from ragna import Rag, source_storages
from ragna.core import Assistant


class ValidationAssistant(Assistant):
    def answer(
        self,
        prompt,
        sources,
        bool_param: bool,
        int_param: int,
        float_param: float,
        string_param: str,
    ):
        pass


async def main():
    async with Rag().chat(
        documents=["README.md"],
        source_storage=source_storages.RagnaDemoSourceStorage,
        assistant=ValidationAssistant,
        unknown=True,
        bool_param=False,
        float_param="string_value",
        string_param=42,
    ):
        pass


import asyncio

asyncio.run(main())

main

pydantic_core._pydantic_core.ValidationError: 4 validation errors for ec32e2f5-ad2d-4d08-8987-609f8e47a6d0
int_param
  Field required [type=missing, input_value={'user': 'philip', 'chat_...ue', 'string_param': 42}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.7/v/missing
float_param
  Input should be a valid number [type=float_type, input_value='string_value', input_type=str]
    For further information visit https://errors.pydantic.dev/2.7/v/float_type
string_param
  Input should be a valid string [type=string_type, input_value=42, input_type=int]
    For further information visit https://errors.pydantic.dev/2.7/v/string_type
unknown
  Extra inputs are not permitted [type=extra_forbidden, input_value=True, input_type=bool]
    For further information visit https://errors.pydantic.dev/2.7/v/extra_forbidden

PR

ragna.core.RagnaException: Validating the Chat parameters resulted in 4 errors:

The following parameters are unknown:

- unknown=True

The following parameters are missing:

- int_param: int

The following parameters have the wrong type:

- string_param: str = 42
- float_param: float = 'string_value'

Copy link
Contributor

@blakerosenthal blakerosenthal left a comment

Choose a reason for hiding this comment

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

This is a little over my head but it fixes a lot of the issues I was having last week

models[(cls, method_name)] = pydantic.create_model( # type: ignore[call-overload]
params = iter(inspect.signature(method).parameters.values())
annotations = get_type_hints(method)
# Skip over the protocol parameters in order for the model below to only
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the main thing that was biting me when trying to add new required parameters to methods on Assistant subclasses. This seems to resolve everything for me, although full disclosure I haven't been able to think through all the nuances of how this might affect custom user components.

}

@contextlib.contextmanager
def _format_validation_error(
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes it way easier to parse all my various user errors, thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants