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

Add support for Pydantic v2 #288

Merged
merged 6 commits into from
Feb 15, 2024
Merged

Conversation

izellevy
Copy link
Collaborator

@izellevy izellevy commented Feb 12, 2024

Problem

Currently we only support Pydantic v1 which is a problem for some users.

Solution

Migrating to Pydantic v2

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Infrastructure change (CI configs, etc)
  • Non-code change (docs, etc)
  • None of the above: (explain here)

Test Plan

Describe specific steps for validating this change.

@izellevy izellevy linked an issue Feb 12, 2024 that may be closed by this pull request
2 tasks
@izellevy izellevy requested a review from acatav February 12, 2024 10:51
Copy link
Collaborator

@igiloh-pinecone igiloh-pinecone left a comment

Choose a reason for hiding this comment

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

@izellevy see a few suggestions

src/canopy/chat_engine/chat_engine.py Show resolved Hide resolved

# In the case of StuffingContextBuilder, we simply want the text representation to
# be a json. Other ContextContent subclasses may render into text differently
def to_text(self, **kwargs):
return self.json(**kwargs)
return json.dumps(self.model_dump(), **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use self.dump_json()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we pass args to json_dumps (such as indent). This was supported in v1 but not v2 (.model_dump_json does not get indent in pydantic v2)

src/canopy/context_engine/context_builder/stuffing.py Outdated Show resolved Hide resolved
src/canopy/llm/models.py Show resolved Hide resolved
@@ -127,7 +127,7 @@ def expected_chunks(documents):
'\ntext in level 3\n#### Level 4\ntext in level 4\n##### Level 5'
'\ntext in level 5\n###### Level 6\ntext in level 6',
source='doc_1',
metadata={'test': '1'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this cast to str before? (Seems like we had a bug?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Kinda bug of Union parsing in pydantic v1. In pydantic v2 there is smart unions so they fixed this problem.

tests/e2e/test_app.py Show resolved Hide resolved
Copy link
Collaborator

@igiloh-pinecone igiloh-pinecone left a comment

Choose a reason for hiding this comment

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

LGTM

@igiloh-pinecone
Copy link
Collaborator

@izellevy please make sure you run tests at least once (locally on your machine) with --all-extras, to make sure we don't have some extra that somehow conflicts with this change

@izellevy
Copy link
Collaborator Author

@igiloh-pinecone checked and made sure it works with all extras

@izellevy izellevy added this pull request to the merge queue Feb 15, 2024
Merged via the queue into pinecone-io:main with commit 04e43dc Feb 15, 2024
7 checks passed
@izellevy izellevy deleted the feature/pydantic-v2 branch February 15, 2024 12:22
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.

[Feature] Update dependency to support Pydantic > 2.0.0
2 participants