Recurring time window filter #51

mrm9084 · 2024-12-11T23:46:10Z

Description

Add the Recurring time window filter to python Feature Management.

Copilot

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 suggestions.

Comments skipped due to low confidence (1)

featuremanagement/_time_window_filter/_recurrence_validator.py:113

The error message should specify the exact parameters for clarity, e.g., 'The Recurrence.Range.EndDate should be after the Start date'.

raise ValueError("The Recurrence.Range.EndDate should be after the Start")

Copilot · 2024-12-11T23:47:19Z

featuremanagement/_time_window_filter/_recurrence_evaluator.py

+    sorted_days_of_week = _sort_days_of_week(pattern.days_of_week, pattern.first_day_of_week)
+    max_day_offset = _get_passed_week_days(sorted_days_of_week[-1], pattern.first_day_of_week)
+    min_day_offset = _get_passed_week_days(sorted_days_of_week[0], pattern.first_day_of_week)
+    num_of_occurrences = number_of_interval * len(sorted_days_of_week) - sorted_days_of_week.index(start.weekday())


The calculation of num_of_occurrences is incorrect. It should be 'number_of_interval * len(sorted_days_of_week) + sorted_days_of_week.index(start.weekday())'.

Suggested change

num_of_occurrences = number_of_interval * len(sorted_days_of_week) - sorted_days_of_week.index(start.weekday())

num_of_occurrences = number_of_interval * len(sorted_days_of_week) + sorted_days_of_week.index(start.weekday())

Pretty sure I have it correct here. https://github.com/microsoft/FeatureManagement-Dotnet/blob/325271bd622aa43d041b366c7ba2e9c30507e5a0/src/Microsoft.FeatureManagement/FeatureFilters/Recurrence/RecurrenceEvaluator.cs#L244, maybe it's the name causing an issue.

featuremanagement/_time_window_filter/_models.py

featuremanagement/_defaultfilters.py

featuremanagement/_time_window_filter/_models.py

zhiyuanliang-ms · 2024-12-12T03:06:39Z

featuremanagement/_time_window_filter/_models.py

+        if range_data.get("EndDate") and isinstance(range_data.get("EndDate"), str):
+            end_date_str = range_data.get("EndDate", "")
+            self.end_date = parsedate_to_datetime(end_date_str) if end_date_str else None
+        self.num_of_occurrences = range_data.get("NumberOfOccurrences")


Missed the default value. BTW, it is possible to get non number value or negative value.

Is the default value 0?

https://github.com/microsoft/FeatureManagement-Dotnet/blob/main/src/Microsoft.FeatureManagement/FeatureFilters/Recurrence/RecurrenceRange.cs#L26

Default value is int.max

Python doesn't have a "Max Int" value. We could do something like math.pow(2, 63) - 1.

zhiyuanliang-ms · 2024-12-12T03:07:52Z

@mrm9084 Will you add testcases in this PR? The testsuite in .NET FM repo or in this PR are very comprehensive.

zhiyuanliang-ms · 2024-12-23T03:25:10Z

featuremanagement/_time_window_filter/_models.py

+            raise ValueError("The interval must be greater than 0.")
+        days_of_week = pattern_data.get("DaysOfWeek", [])
+        for day in days_of_week:
+            self.days_of_week.append(self.days.index(day))


You didn't initialize self.days_of_week as []

zhiyuanliang-ms · 2025-01-06T03:09:52Z

featuremanagement/_defaultfilters.py

@@ -31,6 +41,8 @@
 FEATURE_FILTER_NAME_KEY = "Name"
 IGNORE_CASE_KEY = "ignore_case"

+logger = logging.getLogger(__name__)
+


nit: extra empty line

zhiyuanliang-ms · 2025-01-06T03:29:42Z

featuremanagement/_defaultfilters.py

+        if recurrence:
+            if start_time and end_time:
+                settings = TimeWindowFilterSettings(start_time, end_time, recurrence)
+                return is_match(settings, current_time)


A better timewindow evaluation is

if not start and not end: return False if (start_time is None or start_time <= current_time) and (end_time is None or current_time < end_time): return True if recurrence_data: recurrence = Recurrence(recurrence_data) settings = TimeWindowFilterSettings(start_time, end_time, recurrence) return is_match(settings, current_time) return False

In is_match, you will validate settings (which may take some time), if the current time hits the first time window (specified by start and end), we can save the recurrence evaluation.

zhiyuanliang-ms · 2025-01-06T03:32:12Z

featuremanagement/_time_window_filter/_recurrence_evaluator.py

+    """
+    validate_settings(settings)
+
+    previous_occurrence = _get_previous_occurrence(settings, now)


How about returning False directly here if now < start here. In this way, we can save a function call (very slightly improve the performance).

zhiyuanliang-ms · 2025-01-06T04:54:24Z

featuremanagement/_time_window_filter/_recurrence_evaluator.py

+    pattern_type = settings.recurrence.pattern.type
+    if pattern_type == RecurrencePatternType.DAILY:
+        occurrence_info = _get_daily_previous_occurrence(settings, now)
+    else:


You didn't validate pattern type. Here you assume the pattern type must be either DAILY or WEEKLY. But this is not guaranteed.

How about

if pattern_type == RecurrencePatternType.DAILY: occurrence_info = _get_daily_previous_occurrence(settings, now) elif pattern_type == RecurrencePatternType.WEEKLY: occurrence_info = _get_weekly_previous_occurrence(settings, now) else: throw

zhiyuanliang-ms · 2025-01-06T05:03:11Z

featuremanagement/_time_window_filter/_recurrence_evaluator.py

+    recurrence_range = settings.recurrence.range
+    range_type = recurrence_range.type
+    previous_occurrence = occurrence_info.previous_occurrence
+    end_date = recurrence_range.end_date


Is it a string or datetime type?

zhiyuanliang-ms · 2025-01-06T05:23:05Z

featuremanagement/_time_window_filter/_recurrence_evaluator.py

+
+def _sort_days_of_week(days_of_week: List[int], first_day_of_week: int) -> List[int]:
+    sorted_days = sorted(days_of_week)
+    return sorted_days[sorted_days.index(first_day_of_week) :] + sorted_days[: sorted_days.index(first_day_of_week)]


What will happen if first_day_of_week is not in the list?

zhiyuanliang-ms · 2025-01-06T05:33:26Z

featuremanagement/_time_window_filter/_recurrence_validator.py

+from ._models import RecurrencePatternType, RecurrenceRangeType, TimeWindowFilterSettings
+
+
+DAYS_PER_WEEK = 7


In other files, you used number 7 directly. So maybe we don't need this variable here.

zhiyuanliang-ms · 2025-01-06T05:47:52Z

featuremanagement/_time_window_filter/_models.py

+        for day in days_of_week:
+            self.days_of_week.append(self.days.index(day))
+        first_day_of_week = pattern_data.get("FirstDayOfWeek", "Sunday")
+        self.first_day_of_week = self.days.index(first_day_of_week) if first_day_of_week in self.days else 0


from email.utils import parsedate_to_datetime s = "Sun, 05 Jan 2025 20:00:00 GMT" t = parsedate_to_datetime(s) print(t.weekday())

This produced 6 on my laptop, I am using python 3.11.9.

first day of week should be Sunday by default. Sunday index is 6 and Monday is 0.

mrm9084 added 2 commits November 22, 2024 11:16

added recurring time window filter

d9eca62

Fixing Daily

5339273

mrm9084 requested a review from Copilot December 11, 2024 23:46

Copilot AI reviewed Dec 11, 2024

View reviewed changes

mrm9084 requested review from avanigupta, rossgrambo, albertofori and zhiyuanliang-ms December 11, 2024 23:47

fixing review items

eabad84

zhiyuanliang-ms reviewed Dec 12, 2024

View reviewed changes

featuremanagement/_time_window_filter/_models.py Outdated Show resolved Hide resolved

zhiyuanliang-ms reviewed Dec 12, 2024

View reviewed changes

featuremanagement/_time_window_filter/_models.py Show resolved Hide resolved

zhiyuanliang-ms reviewed Dec 12, 2024

View reviewed changes

mrm9084 added 3 commits December 13, 2024 09:33

Review Comments

1a79afb

Review comments

6fcd070

Fixing days of the week

651eb82

zhiyuanliang-ms reviewed Dec 23, 2024

View reviewed changes

zhiyuanliang-ms reviewed Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recurring time window filter #51

Recurring time window filter #51

mrm9084 commented Dec 11, 2024

Copilot AI left a comment

Copilot AI Dec 11, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

mrm9084 Dec 11, 2024

zhiyuanliang-ms Dec 12, 2024

mrm9084 Dec 13, 2024

zhiyuanliang-ms Dec 16, 2024

mrm9084 Dec 16, 2024

zhiyuanliang-ms commented Dec 12, 2024 •

edited

Loading

zhiyuanliang-ms Dec 23, 2024

zhiyuanliang-ms Jan 6, 2025

zhiyuanliang-ms Jan 6, 2025

zhiyuanliang-ms Jan 6, 2025

zhiyuanliang-ms Jan 6, 2025

zhiyuanliang-ms Jan 6, 2025

zhiyuanliang-ms Jan 6, 2025

zhiyuanliang-ms Jan 6, 2025

zhiyuanliang-ms Jan 6, 2025

	num_of_occurrences = number_of_interval * len(sorted_days_of_week) - sorted_days_of_week.index(start.weekday())
	num_of_occurrences = number_of_interval * len(sorted_days_of_week) + sorted_days_of_week.index(start.weekday())

		from ._models import RecurrencePatternType, RecurrenceRangeType, TimeWindowFilterSettings


		DAYS_PER_WEEK = 7

Recurring time window filter #51

Are you sure you want to change the base?

Recurring time window filter #51

Conversation

mrm9084 commented Dec 11, 2024

Description

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhiyuanliang-ms commented Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhiyuanliang-ms commented Dec 12, 2024 •

edited

Loading