Skip to content

chore: Issue warning announcing entity's value_type as mandatory #4833

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.

Sign up for GitHub

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

Merged
dmartinol merged 2 commits into from
Dec 11, 2024
Merged

chore: Issue warning announcing entity's value_type as mandatory #4833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5160b2
feat: Make entity value_type mandatory with deprecation warning
devin-ai-integration[bot] Dec 11, 2024
5338764
style: Fix import sorting in entity files
devin-ai-integration[bot] Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions sdk/python/feast/entity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import warnings
from datetime import datetime
from typing import Dict, List, Optional

Expand Down Expand Up @@ -79,6 +80,13 @@ def __init__(
ValueError: Parameters are specified incorrectly.
"""
self.name = name
if value_type is None:
warnings.warn(
"Entity value_type will be mandatory in the next release. "
"Please specify a value_type for entity '%s'." % name,
DeprecationWarning,

@dmartinol dmartinol Dec 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not FutureWarning instead?
https://docs.python.org/3/library/exceptions.html#FutureWarning

@franciscojavierarceo franciscojavierarceo Dec 11, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

DeprecationWarning:

Base class for warnings about deprecated features when those warnings are intended for other Python developers.

Ignored by the default warning filters, except in the main module (PEP 565). Enabling the Python Development Mode shows this warning.

FutureWarning

Base class for warnings about deprecated features when those warnings are intended for end users of applications that are written in Python.

I think DeprecationWarning is a reasonable choice but I could see both ways. Devin chose the former. 🤷

@dmartinol dmartinol Dec 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can’t see any deprecation actually, but if Devin will become the main maintainer, that’s fine 🤷🏻‍♂️

@franciscojavierarceo franciscojavierarceo Dec 11, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm going to force it next release.

stacklevel=2,
)
self.value_type = value_type or ValueType.UNKNOWN

if join_keys and len(join_keys) > 1:
Expand Down
15 changes: 15 additions & 0 deletions sdk/python/tests/unit/test_entity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import warnings

import assertpy
import pytest

Expand Down Expand Up @@ -73,3 +75,16 @@ def test_hash():

s4 = {entity1, entity2, entity3, entity4}
assert len(s4) == 3


def test_entity_without_value_type_warns():
with pytest.warns(DeprecationWarning, match="Entity value_type will be mandatory"):
entity = Entity(name="my-entity")
assert entity.value_type == ValueType.UNKNOWN


def test_entity_with_value_type_no_warning():
with warnings.catch_warnings():
warnings.simplefilter("error")
entity = Entity(name="my-entity", value_type=ValueType.STRING)
assert entity.value_type == ValueType.STRING