Skip to content
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.

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

feat: Add more powerful drift windowing strategies, warmup and dynamic thresholds #564

Merged

Conversation

robinholzi
Copy link
Collaborator

@robinholzi robinholzi commented Jul 4, 2024

Motivation

Allow for more powerful drift windowing, dynamic drift threshold as well as a warmup period (for calibration)

@robinholzi robinholzi self-assigned this Jul 4, 2024
Copy link

github-actions bot commented Jul 4, 2024

Line Coverage: -% ( % to main)
Branch Coverage: -% ( % to main)

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

Attention: Patch coverage is 87.82895% with 37 lines in your changes missing coverage. Please review.

Project coverage is 84.86%. Comparing base (3de6590) to head (9594453).

Files Patch % Lines
.../internal/triggers/drift/detection_window/time_.py 50.00% 18 Missing ⚠️
...n/supervisor/internal/triggers/datadrifttrigger.py 86.36% 12 Missing ⚠️
...internal/triggers/drift/detection_window/amount.py 90.00% 3 Missing ⚠️
modyn/supervisor/internal/triggers/utils/utils.py 66.66% 2 Missing ⚠️
...dyn/config/schema/pipeline/trigger/drift/config.py 91.66% 1 Missing ⚠️
...pervisor/internal/triggers/drift/detector/alibi.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #564      +/-   ##
==========================================
- Coverage   84.87%   84.86%   -0.01%     
==========================================
  Files         224      233       +9     
  Lines       10300    10480     +180     
==========================================
+ Hits         8742     8894     +152     
- Misses       1558     1586      +28     

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

@robinholzi
Copy link
Collaborator Author

@MaxiBoether fyi. this should be ready now :)
** I haven't run a pipeline with this yet, so maybe we still need some minor tweaks in a followup PR.

@robinholzi robinholzi requested a review from MaxiBoether July 26, 2024 21:55
@robinholzi robinholzi marked this pull request as ready for review July 26, 2024 21:55
@MaxiBoether MaxiBoether changed the title feat: Add more powerful windowing strategies, warumup and dynamic thresholds feat: Add more powerful windowing strategies, warmup and dynamic thresholds Aug 5, 2024
@MaxiBoether MaxiBoether changed the title feat: Add more powerful windowing strategies, warmup and dynamic thresholds feat: Add more powerful drift windowing strategies, warmup and dynamic thresholds Aug 6, 2024
Copy link
Contributor

@MaxiBoether MaxiBoether left a comment

Choose a reason for hiding this comment

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

Thank you, Robin. I did a first pass. While I did not fully grasp everything yet, I think my major feedback for the next iteration of this PR is that we should think about the interactions between the different components and potentially document them in some README.

In particular, we have aggregation logic for decisions in the pydantic module, we have the drift detectors and then some duplicated decision logic in the decision engine. I think we want to consolidate the how we make drift/no drift decisions in a central place somehow. I also propose to reconsider the naming of the windowing manager. Last, I also encountered some duplication in the new pydantic classes.

I hope my comments make sense. Basically I think the naming and levels of abstraction need some adjustment. Happy to discuss tomorrow or via Github comments!

@robinholzi robinholzi requested a review from MaxiBoether August 11, 2024 10:36
Copy link
Contributor

@MaxiBoether MaxiBoether left a comment

Choose a reason for hiding this comment

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

Thank you, Robin! This is looking good. Most of my comments are nitpicky, I had a couple of smaller questions in some parts

docs/pipeline/triggering/DRIFT_TRIGGERS.md Outdated Show resolved Hide resolved
docs/pipeline/triggering/DRIFT_TRIGGERS.md Outdated Show resolved Hide resolved
modyn/config/schema/pipeline/trigger/drift/config.py Outdated Show resolved Hide resolved
modyn/supervisor/internal/triggers/datadrifttrigger.py Outdated Show resolved Hide resolved
modyn/supervisor/internal/triggers/datadrifttrigger.py Outdated Show resolved Hide resolved
docs/pipeline/triggering/DRIFT_TRIGGERS.md Outdated Show resolved Hide resolved
modyn/supervisor/internal/triggers/datadrifttrigger.py Outdated Show resolved Hide resolved
@robinholzi robinholzi requested a review from MaxiBoether August 13, 2024 08:18
Copy link
Contributor

@MaxiBoether MaxiBoether left a comment

Choose a reason for hiding this comment

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

Thank you!! Some last nitpicky comments, feel free to merge afterwards.

@robinholzi robinholzi force-pushed the robinholzi/feat/more-powerful-drift-windows-and-warmup branch from b870af1 to ea036c1 Compare August 14, 2024 11:01
@robinholzi robinholzi merged commit ec161fa into main Aug 14, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants