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

new: convert nan and inf in local mode to none #603

Merged
merged 5 commits into from
Apr 19, 2024

Conversation

joein
Copy link
Member

@joein joein commented Apr 17, 2024

We forbid to add nan values to the payload in local mode, since the server does not allow to have them.
An attempt to send a request with nan values fails.

However, as of pydantic 2.7, nan values are converted to null, and such requests do not fail.
pydantic/pydantic#8440
pydantic/pydantic-core#1251

Even if we'd like to support nan values in local mode when pydantic>=2.7 is installed, there is no easy way to convert nan values to null in local mode.

This PR suggests to convert nan and inf to null and aligns local mode with pydantic>=2.7

This PR suggests to disable such tests with the remote mode, we'll have a discrepancy then: local mode won't support nan values, server mode won't support them with pydantic<2.7, and will support them with pydantic>=2.7.

One of the alternatives is to convert all the nans and infs to None manually, e.g. with

def convert_nan_inf_to_null(obj):
    if isinstance(obj, dict):
        return {k: convert_nan_inf_to_null(v) for k, v in obj.items()}
    elif isinstance(obj, list):
        return [convert_nan_inf_to_null(v) for v in obj]
    elif isinstance(obj, float):
        if np.isnan(obj) or np.isinf(obj):
            return None
    return obj

Another alternative which I also considered is to extend all the models, e.g. from

class VectorParams(BaseModel, extra="forbid"):
    ...

to

class VectorParams(BaseModel, extra="forbid", allow_inf_nan=False):
    ...

However, it won't work since we use construct and validation does not happen

@joein joein changed the title tests: update nan payload tests with pydantic 2.7 tests: disable nan payload tests with pydantic 2.7 Apr 17, 2024
Copy link

netlify bot commented Apr 17, 2024

Deploy Preview for poetic-froyo-8baba7 ready!

Name Link
🔨 Latest commit c23d52b
🔍 Latest deploy log https://app.netlify.com/sites/poetic-froyo-8baba7/deploys/6622499e41ec330008ae5974
😎 Deploy Preview https://deploy-preview-603--poetic-froyo-8baba7.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@joein joein changed the title tests: disable nan payload tests with pydantic 2.7 tests: convert nan and inf in local mode to none Apr 17, 2024
@joein joein changed the title tests: convert nan and inf in local mode to none new: convert nan and inf in local mode to none Apr 17, 2024
@joein joein requested a review from generall April 17, 2024 16:32
@joein joein force-pushed the pydantic-2.7-convert-nan-to-none branch from a56a4b5 to 477371d Compare April 19, 2024 10:36
@joein joein merged commit 96db386 into dev Apr 19, 2024
14 checks passed
joein added a commit that referenced this pull request Apr 22, 2024
* tests: update nan payload tests with pydantic 2.7

* fix: update local mode to mimic pydantic>=2.7.0

* fix: fix mypy

* fix: do not convert bytes

* fix: allow nan values in payload in local mode
@generall generall deleted the pydantic-2.7-convert-nan-to-none branch May 3, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants