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

Rename BanditData fields from model_ -> bandit_model_ prefix#58

Merged
lisaah merged 4 commits intomainfrom
lisa/ff-2537-python-sdk-using-protected-class-names-that-trigger-pydantic
Jul 3, 2024
Merged

Rename BanditData fields from model_ -> bandit_model_ prefix#58
lisaah merged 4 commits intomainfrom
lisa/ff-2537-python-sdk-using-protected-class-names-that-trigger-pydantic

Conversation

@lisaah
Copy link
Contributor

@lisaah lisaah commented Jul 3, 2024

Fixes: #FF-2537

Motivation and Context

Fields prefixed with model_ are throwing Pydantic warnings like:

/opt/homebrew/lib/python3.11/site-packages/pydantic/_internal/_fields.py:161: UserWarning: Field "model_name" has conflict with protected namespace "model_".

You may be able to resolve this warning by setting `model_config['protected_namespaces'] = ()`.
  warnings.warn(
/opt/homebrew/lib/python3.11/site-packages/pydantic/_internal/_fields.py:161: UserWarning: Field "model_version" has conflict with protected namespace "model_".

You may be able to resolve this warning by setting `model_config['protected_namespaces'] = ()`.
  warnings.warn(
/opt/homebrew/lib/python3.11/site-packages/pydantic/_internal/_fields.py:161: UserWarning: Field "model_data" has conflict with protected namespace "model_".

You may be able to resolve this warning by setting `model_config['protected_namespaces'] = ()`.

Description

Renamed model_ fields to bandit_model

How has this been tested?

Reproducing errors locally, renaming model fields and seeing it without those validation warnings.

class BanditData(SdkBaseModel):
bandit_key: str
model_name: str
bandit_model_name: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

You will need to provide an alias for these fields so that Pydantic knows what property names to use when deserializing from JSON (example payload)

Copy link
Collaborator

Choose a reason for hiding this comment

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

subject["subjectKey"],
ContextAttributes(
numeric_attributes=subject["subjectAttributes"]["numeric_attributes"],
numeric_attributes=subject["subjectAttributes"]["numericAttributes"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

Copy link

@giorgiomartini0 giorgiomartini0 left a comment

Choose a reason for hiding this comment

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

Code change looks good. Need to understand the test failure before merging - are some of the attributes not being processed correctly, leading to a different bandit assignment? Or did the upstream test data change?

@typotter
Copy link
Collaborator

typotter commented Jul 3, 2024

Code change looks good. Need to understand the test failure before merging - are some of the attributes not being processed correctly, leading to a different bandit assignment? Or did the upstream test data change?

Python did not get updated with empty-action-list behaviour. The fix for that is not super straightforward. This PR should be OK to merge with the fix for FF-2573 coming after.
https://linear.app/eppo/issue/FF-2573/python-sdk-do-not-evaluate-string-assignment-or-bandit-if-there-are-no

Copy link
Collaborator

@typotter typotter left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on this!

class BanditData(SdkBaseModel):
bandit_key: str
model_name: str
bandit_model_name: str = Field(alias="modelName")
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

@lisaah lisaah merged commit c015c4f into main Jul 3, 2024
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.

3 participants