-
Notifications
You must be signed in to change notification settings - Fork 30
refactor: task handlers #355
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
refactor: task handlers #355
Conversation
b9b6870
to
824824e
Compare
26c040a
to
9807fb0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 Nice, what a net negative diff! Couple of small comments and generally want to confirm:
(1) functional tests have been run against this change and still pass
(2) telemetry traces are expected [as noted separately]
Thanks @evaline-ju. I'm working on addressing your comments inline and diving into the tracing issue, which will require some changes to fix. We do still need to run the functional tests, so it would be good if someone can help with this in the meantime while I work on the tracing, or it can wait until after that is resolved if you prefer. |
855776f
to
e084c67
Compare
…dlers, add DetectionWarning convenience methods Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
…tions Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Co-authored-by: Evaline Ju <[email protected]> Signed-off-by: Dan Clark <[email protected]>
e084c67
to
6b3915f
Compare
Signed-off-by: declark1 <[email protected]>
6b3915f
to
cb2cbca
Compare
…o spawned tasks (wip) Signed-off-by: declark1 <[email protected]>
b3e195f
to
8bda45e
Compare
…ethods, update conditional logic Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
I made some additional changes for tracing and this is now ready for final review. The primary issue was that span context was not being injected into spawned sub-tasks. A bug fix was also implemented to ensure proper handling of no detectors scenarios in (streaming)_classification_with_gen. While testing tracing further locally with jaeger, I noticed that we were creating an excessive number of spans as essentially every function in the call stack was instrumented and creating a new span, rather than spans corresponding to a "unit of work". We were also instrumenting some functions that didn't need to be. I did some limited cleanup to:
There are opportunities for further enhancements, but we can address them later as this is out of the scope of this PR. |
Signed-off-by: declark1 <[email protected]>
1d013f6
to
eec8c3f
Compare
…to full Signed-off-by: declark1 <[email protected]>
Signed-off-by: declark1 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for the updates!
Major changes:
Result<T, Error>
through tasksOther Changes:
apply_chunk_offset
parameter and fix offsets fordetect_text_contents
taskdetector_id
in client helpers instead of tasksinput_id
from common detector tasks excepttext_contents_detections
(not needed)Option
fromDetectorConfig
fields to improve ergonomicsDetection
/Detections
conversionscurrent_timestamp
util functionTask
struct (not yet used, will adopt in next PR as there are cleanups needed first for request types)None
instead ofSome(ChatDetections { ..default })
when there are no detections, update test accordingly