Skip to content

Commit 97b0f25

Browse files
saket3395saket3395
saket3395
and
saket3395
authored
fix: Feast apply silently ignoring ttl updates to None or timedelta(0) (#6709)
* fix: feast apply ignores ttl updates when new ttl is None or timedelta(0) Fixes #6703. Registry._update_metadata_fields() routes ttl changes on re-apply through a truthiness check: if (... and updated_fv.ttl): None and timedelta(0) are both the documented way to express "no ttl", and both are falsy, so re-applying a FeatureView/LabelView with ttl cleared silently kept the old finite ttl -- feast apply reported the update but nothing changed in the registry. Removing that outer truthy gate isn't sufficient by itself: for the FeatureView branch, get_ttl_duration() returns Python None when self.ttl is None, and the existing inner check if ttl_duration: existing_proto.spec.ttl.CopyFrom(ttl_duration) would still silently skip CopyFrom in that case, leaving the stale ttl in place. Fixed both by explicitly writing an empty Duration() (which decodes back to timedelta(0), per FeatureView.from_proto's existing ToNanoseconds()==0 check) whenever there's no real ttl to write, instead of skipping the write. The LabelView branch is adjusted the same way, guarding FromTimedelta() against a None ttl now that the outer gate no longer prevents ttl=None from reaching this branch. Traced all three cases (None, timedelta(0), a finite value) through the new branch logic in isolation and confirmed FeatureView and LabelView now resolve identically: None and timedelta(0) both produce a zero Duration, a finite ttl passes through unchanged. Signed-off-by: saket3395 <sakettulsan95@gmail.com> * address review nitpicks: hoist Duration import, guard FromTimedelta - Move the Duration import to the top of the file with the other google.protobuf imports, consistent with how Message and RepeatedCompositeFieldContainer are already imported there. - Wrap FromTimedelta() in the LabelView branch with a try/except, re-raising as a ValueError naming the offending value, so an invalid timedelta surfaces a clear error instead of a raw protobuf exception. Signed-off-by: saket3395 <sakettulsan95@gmail.com> * test: cover ttl clearing in _update_metadata_fields (#6703) Adds unit coverage requested in review: - clearing a finite ttl to None or timedelta(0) now writes a zero Duration (previously silently dropped) - a finite-to-finite ttl update is preserved _update_metadata_fields uses no instance state, so it is exercised directly via the class, mirroring the FeatureView/FileSource/Field construction used elsewhere in the unit tests. Signed-off-by: saket3395 <sakettulsan95@gmail.com> * style: apply ruff format to registry.py ttl block Collapse the multi-line raise ValueError back to a single line per ruff format (it fits within the 88-char limit), fixing the format check. Signed-off-by: saket3395 <sakettulsan95@gmail.com> --------- Signed-off-by: saket3395 <sakettulsan95@gmail.com> Co-authored-by: saket3395 <sakettulsan95@gmail.com>
1 parent 7278dcf commit 97b0f25

2 files changed

Lines changed: 67 additions & 10 deletions

File tree

‎sdk/python/feast/infra/registry/registry.py‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
from typing import Any, Dict, List, Optional, Union
2020
from urllib.parse import urlparse
2121

22+
from google.protobuf.duration_pb2 import Duration
2223
from google.protobuf.internal.containers import RepeatedCompositeFieldContainer
2324
from google.protobuf.message import Message
2425

@@ -580,20 +581,23 @@ def _update_metadata_fields(
580581
existing_proto.spec.version = getattr(updated_fv, "version")
581582

582583
# Configuration fields (FeatureView / LabelView TTL)
583-
if (
584-
hasattr(existing_proto.spec, "ttl")
585-
and hasattr(updated_fv, "ttl")
586-
and updated_fv.ttl
587-
):
584+
# Note: don't gate this on `updated_fv.ttl` being truthy -- None and
585+
# timedelta(0) are both the documented way to express "no ttl", and
586+
# are falsy, so that check would silently drop the update exactly
587+
# when a user clears an existing ttl.
588+
if hasattr(existing_proto.spec, "ttl") and hasattr(updated_fv, "ttl"):
588589
if isinstance(updated_fv, FeatureView):
589590
ttl_duration = updated_fv.get_ttl_duration()
590-
if ttl_duration:
591-
existing_proto.spec.ttl.CopyFrom(ttl_duration)
591+
existing_proto.spec.ttl.CopyFrom(
592+
ttl_duration if ttl_duration is not None else Duration()
593+
)
592594
elif isinstance(updated_fv, LabelView):
593-
from google.protobuf.duration_pb2 import Duration
594-
595595
ttl_duration = Duration()
596-
ttl_duration.FromTimedelta(updated_fv.ttl)
596+
if updated_fv.ttl is not None:
597+
try:
598+
ttl_duration.FromTimedelta(updated_fv.ttl)
599+
except (ValueError, OverflowError) as e:
600+
raise ValueError(f"Invalid TTL value: {updated_fv.ttl}") from e
597601
existing_proto.spec.ttl.CopyFrom(ttl_duration)
598602
if hasattr(existing_proto.spec, "online") and hasattr(updated_fv, "online"):
599603
existing_proto.spec.online = getattr(updated_fv, "online")

‎sdk/python/tests/unit/infra/registry/test_update_metadata_fields.py‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
"""Unit tests for Registry._update_metadata_fields TTL handling (issue #6703).
2+
3+
Re-applying a FeatureView with its ttl cleared to ``None`` or
4+
``timedelta(0)`` (both the documented ways to express "no ttl") used to be
5+
silently dropped, because the update was gated on ``updated_fv.ttl`` being
6+
truthy -- and both of those values are falsy.
7+
8+
``_update_metadata_fields`` does not use any instance state, so it is exercised
9+
directly via the class here rather than standing up a full registry backend.
10+
"""
11+
12+
from datetime import timedelta
13+
14+
import pytest
15+
16+
from feast.entity import Entity
17+
from feast.feature_view import FeatureView
18+
from feast.field import Field
19+
from feast.infra.offline_stores.file_source import FileSource
20+
from feast.infra.registry.registry import Registry
21+
from feast.types import Float32
22+
23+
24+
def _feature_view(ttl):
25+
return FeatureView(
26+
name="fv",
27+
entities=[Entity(name="e", join_keys=["e_id"])],
28+
schema=[Field(name="f1", dtype=Float32)],
29+
source=FileSource(path="file://feast/*", timestamp_field="ts_col"),
30+
ttl=ttl,
31+
)
32+
33+
34+
@pytest.mark.parametrize("cleared_ttl", [None, timedelta(0)])
35+
def test_update_metadata_fields_clears_ttl(cleared_ttl):
36+
existing_proto = _feature_view(timedelta(days=10)).to_proto()
37+
# sanity: the existing view starts with a finite ttl
38+
assert existing_proto.spec.ttl.ToNanoseconds() != 0
39+
40+
updated_fv = _feature_view(cleared_ttl)
41+
Registry._update_metadata_fields(None, existing_proto, updated_fv)
42+
43+
# the cleared ttl (None / timedelta(0)) must now be reflected as "no ttl"
44+
assert existing_proto.spec.ttl.ToNanoseconds() == 0
45+
46+
47+
def test_update_metadata_fields_preserves_finite_ttl():
48+
existing_proto = _feature_view(timedelta(days=10)).to_proto()
49+
50+
updated_fv = _feature_view(timedelta(days=3))
51+
Registry._update_metadata_fields(None, existing_proto, updated_fv)
52+
53+
assert existing_proto.spec.ttl.ToTimedelta() == timedelta(days=3)

0 commit comments

Comments
 (0)