Skip to content

bug: handle Optional params in schema validation#2980

Merged
ZanSara merged 22 commits intodeepset-ai:mainfrom
anakin87:handle_optional_params
Aug 24, 2022
Merged

bug: handle Optional params in schema validation#2980
ZanSara merged 22 commits intodeepset-ai:mainfrom
anakin87:handle_optional_params

Conversation

@anakin87
Copy link
Member

@anakin87 anakin87 commented Aug 6, 2022

Related Issues

Proposed Changes:

First draft to manually temporarily fix this problem.
When Pydantic v2 will be released, there will be no need of this fix.

How did you test it?

Included test

Notes for the reviewer

Checklist

@anakin87 anakin87 requested a review from a team as a code owner August 6, 2022 15:12
@anakin87 anakin87 marked this pull request as draft August 6, 2022 15:34
@masci masci requested review from masci and removed request for a team August 6, 2022 15:41
@anakin87
Copy link
Member Author

anakin87 commented Aug 8, 2022

Notes for the reviewer

In handle_optional_params function:

  • I first find the optional params. This implementation is tested in Python 3.7 and 3.10, even if the annotations management is different in the two versions.
  • for every optional param, the schema is updated using anyOf and {"type": "null"}.
    I don't like very much this part and I also tried to monkey patch the Pydantic schema generation, but it seemed too intricate.

The test tries to load a pipeline, with a null parameter. This test was failing with the current JSON schema.

@anakin87 anakin87 marked this pull request as ready for review August 8, 2022 10:31
Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

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

Hello @anakin87, thank you, this issue has been standing for a good while!

I found some small occasions for improvement but the approach looks good.

@vblagoje
Copy link
Member

Great timing @ZanSara and @anakin87 - I need this PR for #3062

@anakin87 anakin87 requested review from ZanSara and removed request for masci August 24, 2022 08:00
Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

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

Amazing, thank you! The test is 100% exactly as I had it in mind 😊

@ZanSara ZanSara merged commit 891707e into deepset-ai:main Aug 24, 2022
@anakin87 anakin87 deleted the handle_optional_params branch August 24, 2022 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic:pipeline type:bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Schema validation does not allow for null values in case of Optional typed params

3 participants