Skip to content
This repository was archived by the owner on Nov 8, 2024. It is now read-only.

Conversation

leoromanovsky
Copy link
Member

@leoromanovsky leoromanovsky commented Sep 21, 2023

Fixes: #issue

Motivation and Context

Our customers are upgrading to pydantic 2; we need to in order to unblock them.

Following the migration guide (https://docs.pydantic.dev/2.0/migration/) and used the bump-pydantic tool to generate changes.

Description

How has this been tested?

Unit tests initially failed; continued working on the PR until they were all ✅

@leoromanovsky
Copy link
Member Author

Paired with @sameerank and we worked through some test failures - remaining ones are in client_test.py in the integration tests https://github.com/Eppo-exp/python-sdk/actions/runs/6318854804/job/17158725446?pr=24

@leoromanovsky leoromanovsky force-pushed the lr/ff-957/upgrade-pydantic branch from df689c9 to bebce94 Compare October 5, 2023 02:56
Comment on lines +6 to +7
class AssignmentLogger(BaseModel):
model_config = ConfigDict(arbitrary_types_allowed=True)
Copy link
Member Author

Choose a reason for hiding this comment

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

AssignmentLogger does not inherit from SdkBaseModel because it has unique serialization requirements from the json dto objects

Comment on lines -2 to -8


def to_camel(s: str):
words = s.split("_")
if len(words) > 1:
return words[0] + "".join([w.capitalize() for w in words[1:]])
return words[0]
Copy link
Member Author

Choose a reason for hiding this comment

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

glad they have an official version publish! our to_camel had a bug in it preventing test cases from passing

Comment on lines 5 to +6
class SdkBaseModel(BaseModel):
class Config:
alias_generator = to_camel
allow_population_by_field_name = True
model_config = ConfigDict(alias_generator=to_camel, populate_by_name=True)
Copy link
Member Author

Choose a reason for hiding this comment

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

SdkBaseModel is the parent class of all json dto classes

value=experiment_config.overrides[subject_hash],
typedValue=experiment_config.typedOverrides[subject_hash],
typed_value=experiment_config.typed_overrides[subject_hash],
shard_range=ShardRange(start=0, end=10000),
Copy link
Member Author

Choose a reason for hiding this comment

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

python classes now have snake case, as they're supposed to

name: str
value: str
typedValue: Any
typed_value: Any = None
Copy link
Member Author

Choose a reason for hiding this comment

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

pydantic required this

install_requires =
pydantic<2
pydantic
pydantic-settings
Copy link
Member Author

Choose a reason for hiding this comment

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

why is there a setup.cfg and requirements.txt file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't know what the intention was here, but I believe the convention is about abstract vs concrete dependencies

https://martin-thoma.com/python-requirements/#abstract-dependencies

@leoromanovsky leoromanovsky marked this pull request as ready for review October 5, 2023 04:17
Copy link
Contributor

@schmit schmit left a comment

Choose a reason for hiding this comment

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

Nice, but don't forget to bump the version in __init__.py

@leoromanovsky leoromanovsky merged commit 42830db into main Oct 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants