[AnomalyDetection] Add base classes and specifiable protocol by shunping · Pull Request #33845 · apache/beam

shunping · 2025-02-04T04:08:41Z

This is the first part of the code push for anomaly detection transform.

shunping · 2025-02-04T04:10:58Z

github-actions · 2025-02-04T04:12:14Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

codecov · 2025-02-04T06:11:39Z

Codecov Report

Attention: Patch coverage is 91.16022% with 16 lines in your changes missing coverage. Please review.

Project coverage is 59.12%. Comparing base (5aae10d) to head (bc6fa8f).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
sdks/python/apache_beam/ml/anomaly/base.py	83.60%	10 Missing ⚠️
sdks/python/apache_beam/ml/anomaly/specifiable.py	95.00%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #33845      +/-   ##
============================================
+ Coverage     59.08%   59.12%   +0.03%     
  Complexity     3237     3237              
============================================
  Files          1156     1158       +2     
  Lines        176907   177113     +206     
  Branches       3391     3391              
============================================
+ Hits         104532   104715     +183     
- Misses        69008    69031      +23     
  Partials       3367     3367

Flag	Coverage Δ
python	`81.25% <91.16%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

damccorm

Mostly LGTM, but had some questions. In general, docs would be really helpful here. I really like the concept

sdks/python/apache_beam/ml/anomaly/base.py

sdks/python/apache_beam/ml/anomaly/base_test.py

sdks/python/apache_beam/ml/anomaly/specifiable.py

damccorm · 2025-02-04T19:59:12Z

sdks/python/apache_beam/ml/anomaly/specifiable.py

+    def run_init(self):
+      original_init(self, **self._init_params)
+
+    def new_getattr(self, name):


Maybe I'm not understanding this well, but could we just do something a bit simpler like:

def new_getattr(self, name): if not self._initialized: original_init() self._initialized = True return orig_getattr(self, name)

if we keep track of the original getattr function in orig_getattr

The general idea is like this, but there are some edge cases we need to avoid. Otherwise, we will end up with an infinite loop.

damccorm · 2025-02-04T19:59:43Z

sdks/python/apache_beam/ml/anomaly/specifiable.py

+
+from typing_extensions import Self
+
+ACCEPTED_SPECIFIABLE_SUBSPACES = [


Why do we start with these registered? Would it be cleaner to just register them on import?

These are the accepted subspaces for the known specifiable, but not the actual specifiable classes. The purpose is to avoid using a global space for every specifiable classes.

I think they should be predefined before the true registration takes place. WDYT?

The purpose is to avoid using a global space for every specifiable classes.

I think maybe this is the piece I'm missing - why is this helpful? Aka, why is it helpful to have a nested KNOWN_SPECIFIABLE[subspace][spec_type] dict instead of just KNOWN_SPECIFIABLE[spec_type]. I can't really think of scenarios this enables/makes easier

There is a concern about keeping everything in a global namespace from the design doc.
https://docs.google.com/document/d/1tE8lz9U_vjlNn2H7t-GRrs3vfhQ5UuCgWiHXCRHRPns/edit?disco=AAABaZ332MQ

I think the primary goal here is to avoid naming conflict, but I am open to any suggestion.

Ok, I can live with this I think, lets just make sure:

a) the user has an easy path to create this object without specifying a subspace (e.g. the mainline case where conflicts are not really an issue)
b) it is really clear what a user should do if there's a conflict

(a) Yes, I think the subspace is transparent to users. Users don't need to specify subspace when they use the decorator to augment their class.

(b) When there is a conflict, users can choose to specify a different spec_type when calling the decorator. I will make sure I have added that clearly in the docstring of the decorator.

On a second thought, I think it may be helpful to turn KNOWN_SPECIFIABLE into a module private variable, so users will know they are not supposed to manage it directly.

shunping · 2025-02-07T05:17:12Z

@damccorm, I've added docstrings and refactored some code for readability. PTAL. Thanks!

sdks/python/apache_beam/ml/anomaly/specifiable.py

damccorm · 2025-02-07T16:04:05Z

sdks/python/apache_beam/ml/anomaly/specifiable.py

+
+from typing_extensions import Self
+
+ACCEPTED_SPECIFIABLE_SUBSPACES = [


The purpose is to avoid using a global space for every specifiable classes.

I think maybe this is the piece I'm missing - why is this helpful? Aka, why is it helpful to have a nested KNOWN_SPECIFIABLE[subspace][spec_type] dict instead of just KNOWN_SPECIFIABLE[spec_type]. I can't really think of scenarios this enables/makes easier

…only.

…c_type to resolve naming conclict.

damccorm

Thanks for the updates. I just have one more thought which includes an API change - other than that we can take this and add in things as needed in the future

sdks/python/apache_beam/ml/anomaly/specifiable.py

damccorm

Thanks!

…33845) * Add base classes and specifiable protocol for anomaly detection. * Add subspaces to global specifiable map * Add __init__.py * Fix lints * Fix get_subspace when calling from from_spec * Refactor code, add tests and add docstrings. * Minor changes to docstrings and comments * Remove the fallback subspace '*' from accepted list. Use it in tests only. * Bring fallback subspace back to accepted list. Clarify the use of spec_type to resolve naming conclict. * Make _KNOWN_SPECIFIABLE a defaultdict. Remove error_if_exiists. * Minor adjustment on docstrings.

Add base classes and specifiable protocol for anomaly detection.

18f9656

github-actions bot added the python label Feb 4, 2025

shunping added 2 commits February 4, 2025 00:22

Add subspaces to global specifiable map

3ea1d0a

Add __init__.py

0e6b9a7

shunping changed the title ~~[Anomaly Detection] Add base classes and specifiable protocol~~ [AnomalyDetection] Add base classes and specifiable protocol Feb 4, 2025

shunping force-pushed the anomaly-detection branch from bc6fa8f to acc6448 Compare February 4, 2025 13:25

Fix lints

4023c80

shunping force-pushed the anomaly-detection branch from acc6448 to 4023c80 Compare February 4, 2025 14:56

Fix get_subspace when calling from from_spec

24c77e9

damccorm reviewed Feb 4, 2025

View reviewed changes

shunping marked this pull request as draft February 6, 2025 19:59

shunping force-pushed the anomaly-detection branch 2 times, most recently from 371d465 to 0b3a7ca Compare February 7, 2025 04:45

shunping marked this pull request as ready for review February 7, 2025 04:55

Refactor code, add tests and add docstrings.

e4a32c2

shunping force-pushed the anomaly-detection branch from 0b3a7ca to e4a32c2 Compare February 7, 2025 05:13

damccorm reviewed Feb 7, 2025

View reviewed changes

shunping added 2 commits February 7, 2025 15:31

Minor changes to docstrings and comments

ebfa85e

Remove the fallback subspace '*' from accepted list. Use it in tests …

5f0debf

…only.

shunping force-pushed the anomaly-detection branch from b9221f1 to 5f0debf Compare February 7, 2025 20:38

Bring fallback subspace back to accepted list. Clarify the use of spe…

38b0a89

…c_type to resolve naming conclict.

shunping force-pushed the anomaly-detection branch from 38ae483 to 38b0a89 Compare February 7, 2025 21:34

damccorm reviewed Feb 7, 2025

View reviewed changes

sdks/python/apache_beam/ml/anomaly/specifiable.py Outdated Show resolved Hide resolved

shunping added 2 commits February 7, 2025 19:57

Make _KNOWN_SPECIFIABLE a defaultdict. Remove error_if_exiists.

9dba625

Minor adjustment on docstrings.

fb1d3b3

damccorm approved these changes Feb 10, 2025

View reviewed changes

damccorm merged commit 30a8c2d into apache:master Feb 10, 2025
91 checks passed

This was referenced Feb 15, 2025

[AnomalyDetection] Add univariate trackers #33994

Merged

[AnomalyDetection] Add threshold and aggregation functions. #34018

Merged

shunping mentioned this pull request Jul 27, 2025

[Task]: Add Anomaly Detection PTransform to Beam #35706

Closed

17 tasks


		from typing_extensions import Self

		ACCEPTED_SPECIFIABLE_SUBSPACES = [

Conversation

shunping commented Feb 4, 2025

Uh oh!

shunping commented Feb 4, 2025

Uh oh!

github-actions bot commented Feb 4, 2025

Uh oh!

codecov bot commented Feb 4, 2025

Codecov Report

Uh oh!

damccorm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shunping Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shunping commented Feb 7, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damccorm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

damccorm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shunping Feb 6, 2025 •

edited

Loading