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

Fix validator initialization bugs #553

Merged
merged 9 commits into from
Jan 30, 2024
Merged

Conversation

thekaranacharya
Copy link
Contributor

@thekaranacharya thekaranacharya commented Jan 22, 2024

Bug Description
As found in: #550, error was thrown when using PydanticFieldValidator due to a missing keyword argument in the super.__init__ call within the validator initialization method. This is a bug and all keyword argument(s) that a validator expects should be passed explicitly to the super call as passing in **kwargs doesn't automatically include the remaining keyword arguments.

Turns out, this is a pattern that has been erroneously implemented across the following 6 other validators alongwith the PydanticFieldValidator:

  • BugFreeSQL
  • ExtractedSummarySentencesMatch
  • ExtractiveSummary
  • QARelevanceLLMEval
  • RemoveRedundantSentences
  • SaliencyCheck

Fix
All other validators pass in the keyword arguments explicitly in either of the following two ways:

  1. super.__init__(on_fail, kwarg_1=kwarg_1, kwarg_2=kwarg_2,... **kwargs)
  2. super.__init__(kwarg_1=kwarg_1, kwarg_2=kwarg_2,... on_fail=on_fail, **kwargs)

The super.__init__ method's definition is as follows:

def __init__(self, on_fail: Optional[Union[Callable, str]] = None, **kwargs):

Hence, both of the ways are fine and the arguments get passed in correctly. This PR corrects these 7 validators by using the first way (either is fine)
A relevant similar PR was previously opened for ProvenanceV0: #288

@thekaranacharya thekaranacharya added bug Something isn't working urgent labels Jan 22, 2024
@CalebCourier
Copy link
Collaborator

@thekaranacharya good work on this. I went back and looked at the history of PydanticFieldValidation and it looks like it was an internal wrapper that should now be deprecated. See here and here. If we're in agreement on that, I think we should add a deprecation warning as part of this PR so it can be removed in the next breaking version.

@thekaranacharya
Copy link
Contributor Author

Got it. So basically if users define a PydanticFieldValidator, it will be converted to a Guardrails validator - or is there another way for a user to pass in a validation function and use it similar to the PydanticFieldValidator? We can add in some examples if it's the latter, alongwith the deprecation warning.

Copy link

Validator init bugs

CalebCourier
CalebCourier previously approved these changes Jan 29, 2024
Copy link
Collaborator

@CalebCourier CalebCourier left a comment

Choose a reason for hiding this comment

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

We should fast follow this PR with adding Pydantic 1.x and 2.x examples on the custom validators page: https://www.guardrailsai.com/docs/concepts/validators/#custom-validators

Right now it only shows how to use them in a RAIL spec.

See here for a 1.x example

and here for a 2.x example

@zsimjee zsimjee merged commit bb10b3c into main Jan 30, 2024
60 checks passed
@thekaranacharya thekaranacharya deleted the karan/bugfix-validator-init branch January 31, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants