Change session_metadata_api Session from std lib dataclass to Pydantic #313
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the Session and SessionQueryConfiguration classes from standard library dataclasses to Pydantic BaseModel classes to support issue #307. The change introduces validation capabilities and automatic serialization/deserialization features through Pydantic.
- Converts Session and SessionQueryConfiguration from @DataClass to Pydantic BaseModel
- Removes timestamp and user tracking fields from Session class
- Updates type annotations to use lowercase list instead of List
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # BUSINESS ADVANTAGE OR UNAVAILABILITY, OR LOSS OR CORRUPTION OF | ||
| # DATA. | ||
| # | ||
| import dataclasses |
There was a problem hiding this comment.
[nitpick] The dataclasses import is only used for the UpdatableSession class. Consider removing this import and converting UpdatableSession to Pydantic as well for consistency, or add a comment explaining why UpdatableSession remains a dataclass.
There was a problem hiding this comment.
I think the time it would take (to be fair, minutes) to test changing UpdatableSession too isn't too worth it right now. But maybe I'm being too lazy 😶
| model_config = ConfigDict( | ||
| validate_by_name=True, | ||
| alias_generator=alias_generators.to_camel, | ||
| revalidate_instances="always", |
There was a problem hiding this comment.
Without this, Session.model_validate(session) is little more than a fancy instanceof() check. With it, the types of fields are actually validated.
Probably not necessary everywhere we have Pydantic models at this time, but I need it here for a check in #307.
| updated_by_id: str | ||
| inference_model: str | ||
| rerank_model: str | ||
| rerank_model: Optional[str] |
There was a problem hiding this comment.
It seems rerank_model can come back as None from the Java backend; it was never an error before because dataclasses.dataclass doesn't validate types.
| time_created: datetime | ||
| time_updated: datetime | ||
| created_by_id: str | ||
| updated_by_id: str |
There was a problem hiding this comment.
Removing these because time_created and time_updated were causing a slight pain serializing to and from JSON (also turns out they were floats this whole time), and it doesn't seem like we're actually using these fields anywhere.
There was a problem hiding this comment.
Does removing these raise an error while updating sessions? or it just works fine?
There was a problem hiding this comment.
Great question! I've tested this locally, and encounter no errors when updating a session, nor creating a new one (which calls /rename-session), nor chatting.
We're not actually using these fields when making session update requests anyway (since they're managed at the DB layer), and we're also not using them here in the Python service.
| class Session: | ||
| class Session(BaseModel): | ||
| model_config = ConfigDict( | ||
| alias_generator=alias_generators.to_camel, |
There was a problem hiding this comment.
This allows us to output a camelCased dictionary by calling .model_dump(by_alias=True)!
| class Session(BaseModel): | ||
| model_config = ConfigDict( | ||
| alias_generator=alias_generators.to_camel, | ||
| validate_by_name=True, |
There was a problem hiding this comment.
This is needed to allow fields to still be set by their snake_case names.
mliu-cloudera
force-pushed
the
liu/dataclass
branch
from
c8aaa45 to
8f19395
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import dataclasses | ||
| import json | ||
| from dataclasses import dataclass, field | ||
| from datetime import datetime | ||
| from typing import List, Any, Optional |
There was a problem hiding this comment.
The dataclasses import is only used for the UpdatableSession class. Consider moving this import closer to where it's used or removing it if UpdatableSession will also be migrated to Pydantic.
| disable_streaming=data["queryConfiguration"].get( | ||
| "disableStreaming", False | ||
| ), | ||
| disable_streaming=data["queryConfiguration"].get("disableStreaming", False), |
There was a problem hiding this comment.
[nitpick] This line formatting change appears to be unrelated to the Pydantic migration. Consider keeping the original multi-line format for consistency with the surrounding code style.
2 participants
To help facilitate #307