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(weave): allow disabling autopatch #3442

Merged
merged 2 commits into from
Jan 21, 2025
Merged

Conversation

jamie-rasmussen
Copy link
Collaborator

Description

Gives users the ability to disable autopatching, giving improved weave.init performance by almost a second if the user does not need integrations. An example use: a script that doesn't need to log any traces, like the benchmarking script included here.

For discussion related to #3441

Before: Can disable individual integrations

After: Can also easily disable all integrations

Future?: Can selectively enable integrations

Integrations enabled:

python benchmark_init.py 
Logged in as Weights & Biases user: jamie-rasmussen.
View Weave data at https://wandb.ai/jamie-rasmussen/2025-01-20_benchmark_init/weave
1.981088749999799
1.254892750000181
1.242289083000287
1.322170541999185
1.3297177079994071
1.2651509999996051
1.3684602919993267
1.268961792000482
1.3221507079997536
1.3851784170001338
  Running init tests... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00 0:00:36


 Init Time Benchmark Results  
┏━━━━━━━━━━━━━━━━━━┳━━━━━━━━━┓
┃ Metric           ┃ Value   ┃
┡━━━━━━━━━━━━━━━━━━╇━━━━━━━━━┩
│ Mean init time   │ 1.3740s │
│ Median init time │ 1.3222s │
│ Std dev          │ 0.2187s │
│ Min time         │ 1.2423s │
│ Max time         │ 1.9811s │
└──────────────────┴─────────┘


  Individual Init Times   
┏━━━━━━━┳━━━━━━━━━━━━━━━━┓
┃ Run # ┃ Time (seconds) ┃
┡━━━━━━━╇━━━━━━━━━━━━━━━━┩
│ 1     │ 1.9811         │
│ 2     │ 1.2549         │
│ 3     │ 1.2423         │
│ 4     │ 1.3222         │
│ 5     │ 1.3297         │
│ 6     │ 1.2652         │
│ 7     │ 1.3685         │
│ 8     │ 1.2690         │
│ 9     │ 1.3222         │
│ 10    │ 1.3852         │
└───────┴────────────────┘

Integrations disabled:

python benchmark_init.py --disable-autopatch
Logged in as Weights & Biases user: jamie-rasmussen.
View Weave data at https://wandb.ai/jamie-rasmussen/2025-01-20_benchmark_init/weave
0.45323512499999197
0.48261237500082643
0.467393875000198
0.540551707999839
0.4925498749998951
0.44786283299981733
0.4837395419999666
0.4726712920000864
0.38254491699990467
0.5333322499991482
  Running init tests... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00 0:00:25


 Init Time Benchmark Results  
┏━━━━━━━━━━━━━━━━━━┳━━━━━━━━━┓
┃ Metric           ┃ Value   ┃
┡━━━━━━━━━━━━━━━━━━╇━━━━━━━━━┩
│ Mean init time   │ 0.4756s │
│ Median init time │ 0.4776s │
│ Std dev          │ 0.0447s │
│ Min time         │ 0.3825s │
│ Max time         │ 0.5406s │
└──────────────────┴─────────┘


  Individual Init Times   
┏━━━━━━━┳━━━━━━━━━━━━━━━━┓
┃ Run # ┃ Time (seconds) ┃
┡━━━━━━━╇━━━━━━━━━━━━━━━━┩
│ 1     │ 0.4532         │
│ 2     │ 0.4826         │
│ 3     │ 0.4674         │
│ 4     │ 0.5406         │
│ 5     │ 0.4925         │
│ 6     │ 0.4479         │
│ 7     │ 0.4837         │
│ 8     │ 0.4727         │
│ 9     │ 0.3825         │
│ 10    │ 0.5333         │
└───────┴────────────────┘

@jamie-rasmussen jamie-rasmussen force-pushed the jamie/disable_autopatch branch from c3a599d to 290c710 Compare January 20, 2025 17:00
@circle-job-mirror
Copy link

circle-job-mirror bot commented Jan 20, 2025

@jamie-rasmussen jamie-rasmussen marked this pull request as ready for review January 20, 2025 21:41
@jamie-rasmussen jamie-rasmussen requested a review from a team as a code owner January 20, 2025 21:41
@@ -51,6 +52,11 @@ class AutopatchSettings(BaseModel):

@validate_call
def autopatch(settings: Optional[AutopatchSettings] = None) -> None:
if settings is None:
settings = AutopatchSettings()
elif settings.disable_autopatch:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: would just do if here

Copy link
Collaborator

@andrewtruong andrewtruong left a comment

Choose a reason for hiding this comment

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

Even better would be deferring autopatch until the first import so we don't even need this!

@jamie-rasmussen
Copy link
Collaborator Author

Even better would be deferring autopatch until the first import so we don't even need this!

That would be better but also a lot more complicated to implement.

@jamie-rasmussen jamie-rasmussen force-pushed the jamie/disable_autopatch branch from 290c710 to f77fa9a Compare January 21, 2025 15:39
@jamie-rasmussen jamie-rasmussen merged commit 540cf68 into master Jan 21, 2025
128 checks passed
@jamie-rasmussen jamie-rasmussen deleted the jamie/disable_autopatch branch January 21, 2025 17:06
@github-actions github-actions bot locked and limited conversation to collaborators Jan 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants