[OPIK-99] Implement batching for spans creation #298

alexkuzmik · 2024-09-20T16:00:57Z

Details

Added batching for CREATE-SPAN requests.
Batches are flushed to the streamer queue automatically every second or whenever accumulated message items reach max batch size limit (at the moment - 1000 items).

Batching works only inside the SDK, if someone tries to do the following

import opik
client = opik.Opik()

... batching will not work for that client (and all clients derived from it).

Under the hood of the SDK we instantiate Opik client like that

client = opik.Opik(_use_batching=True)

Issues

Resolves the issue when SDK sends a lot of CREATE-SPAN requests to the backend independetly, loading it and utilizing more network resources.
Backend should be less loaded,

Testing

Added unit tests. E2E tests also pass, but this PR requires extra QA.

Documentation

andrescrz

One comment, but mostly LGTM. I had maybe some comment about optimising the batcher lock, but I don't want to create additional noise.

Once this is reviewed and validated, from my side we're good to go.

sdks/python/src/opik/message_processing/batching/base_batcher.py

andrescrz

As discussed, let's debug why span name is none with this change before proceeding with the merge.

andrescrz

Just a few more comments while the discussed items are implemented.

andrescrz · 2024-09-25T09:54:30Z

.github/workflows/sdk-e2e-tests.yaml

@@ -34,7 +34,7 @@ jobs:
        - name: Run latest Opik server
          run: |
            cd deployment/docker-compose
-            docker compose up -d
+            docker compose up -d --build --force-recreate --no-deps


Let's double check if these flags --force-recreate --no-deps were just a temporary fix or if we need it in the long run:

Dummy commit #309

If the later, we need to propagate this everywhere.

@thiagohora

The issue is that --build only builds if the tag specified doesn't exist yet. However, all PR seems to run with the value.

https://github.com/comet-ml/opik/blob/main/deployment/docker-compose/docker-compose.yaml#L61

This forces the rebuid of the image/tag

It will conflict with this PR:

build opik server while runing it for tests #310
Please @liyaka leave your thoughts here.

.github/workflows/sdk-e2e-tests.yaml

andrescrz · 2024-09-25T09:55:09Z

deployment/docker-compose/docker-compose.yaml

+      args:
+        OPIK_VERSION: ${OPIK_VERSION:-latest}


Same, let's double check if this was temporary.

@thiagohora ? I thought it's a part of the fix.

Yeah, shit seems to be missing in our docker image. We such a variable.

@liyaka please see this thread above.

sdks/python/src/opik/message_processing/batching/batch_manager_constuctors.py

sdks/python/src/opik/message_processing/batching/flushing_thread.py

andrescrz

Let's just wait on some input from @liyaka for the build fixes. Anything else LGTM.

andrescrz · 2024-09-26T07:49:36Z

.github/workflows/sdk-e2e-tests.yaml

@@ -34,7 +34,7 @@ jobs:
        - name: Run latest Opik server
          run: |
            cd deployment/docker-compose
-            docker compose up -d
+            docker compose up -d --build --force-recreate --no-deps


It will conflict with this PR:

build opik server while runing it for tests #310
Please @liyaka leave your thoughts here.

andrescrz · 2024-09-26T07:50:59Z

deployment/docker-compose/docker-compose.yaml

+      args:
+        OPIK_VERSION: ${OPIK_VERSION:-latest}


@liyaka please see this thread above.

…to use batching

…add tests for span batcher

…cessors.py

…anually in Opik clients created under the hood of SDK

andrescrz

Undone the e2e infra changes as they conflict and they should tackled separately.

* Batch processing draft * Batching draft * [DRAFT] Update backend emulator for tests, update message processing to use batching * Fix lint errors * Implement _process_create_span_batch_message method in MessageProcessor * Remove assert, add debug log message * Add base batcher unit test * Make batch manager flush all batches if streamer.flush() was called, add tests for span batcher * Add tests for batch manager * Add test for flushing thread * Add one more test for batch manager * Fix lint errors * Fix lint errors * Fix bug when backend emulator didn't add feedback scores to traces * Fix lint errors * Rename flush_interval to flush_interval_seconds * Enable debug logs for e2e tests. Update debug messages in message_processors.py * Remove export statement from e2e workflow * Enable backend build. Enable debug logs for e2e tests * Update docker compose and e2e workflow file * Make batching disabled by default in Opik client. It is now enabled manually in Opik clients created under the hood of SDK * Add more unit tests for message processing * Add docstring for _use_batching parameter * Add missing _SECONDS suffix * Rename constant * Undone e2e tests infra changes --------- Co-authored-by: Andres Cruz <[email protected]>

alexkuzmik requested a review from a team as a code owner September 20, 2024 16:00

alexkuzmik self-assigned this Sep 20, 2024

alexkuzmik requested a review from japdubengsub September 20, 2024 16:01

andrescrz reviewed Sep 23, 2024

View reviewed changes

sdks/python/src/opik/message_processing/batching/base_batcher.py Outdated Show resolved Hide resolved

japdubengsub previously approved these changes Sep 24, 2024

View reviewed changes

alexkuzmik dismissed japdubengsub’s stale review via 11eef36 September 24, 2024 14:41

thiagohora previously approved these changes Sep 24, 2024

View reviewed changes

andrescrz requested changes Sep 25, 2024

View reviewed changes

andrescrz reviewed Sep 25, 2024

View reviewed changes

alexkuzmik dismissed thiagohora’s stale review via df3f641 September 25, 2024 16:36

andrescrz reviewed Sep 26, 2024

View reviewed changes

alexkuzmik added 19 commits September 30, 2024 15:58

Batch processing draft

eeccd8e

Batching draft

a6420ce

[DRAFT] Update backend emulator for tests, update message processing …

97e8c97

…to use batching

Fix lint errors

1ca51c6

Implement _process_create_span_batch_message method in MessageProcessor

87631cc

Remove assert, add debug log message

67af3f3

Add base batcher unit test

bba3418

Make batch manager flush all batches if streamer.flush() was called, …

4716854

…add tests for span batcher

Add tests for batch manager

c9f5ce3

Add test for flushing thread

aedca86

Add one more test for batch manager

215fba3

Fix lint errors

318b981

Fix lint errors

2a2fac9

Fix bug when backend emulator didn't add feedback scores to traces

48e6319

Fix lint errors

52cc100

Rename flush_interval to flush_interval_seconds

95fe89f

Enable debug logs for e2e tests. Update debug messages in message_pro…

21dc3c6

…cessors.py

Remove export statement from e2e workflow

47f7007

Enable backend build. Enable debug logs for e2e tests

e48eac2

alexkuzmik added 6 commits September 30, 2024 15:58

Update docker compose and e2e workflow file

9282dfc

Make batching disabled by default in Opik client. It is now enabled m…

587f12a

…anually in Opik clients created under the hood of SDK

Add more unit tests for message processing

ed5dcf1

Add docstring for _use_batching parameter

587d5bd

Add missing _SECONDS suffix

294c81f

Rename constant

ed29744

andrescrz force-pushed the OPIK-99-sdk-implement-batching-for-spans-creation branch from 6357040 to ed29744 Compare September 30, 2024 13:59

This comment was marked as outdated.

Sign in to view

Undone e2e tests infra changes

944fa9d

andrescrz dismissed their stale review via 944fa9d September 30, 2024 14:11

andrescrz force-pushed the OPIK-99-sdk-implement-batching-for-spans-creation branch from 9d5d062 to 944fa9d Compare September 30, 2024 14:11

andrescrz approved these changes Sep 30, 2024

View reviewed changes

Nimrod007 approved these changes Sep 30, 2024

View reviewed changes

andrescrz merged commit 939accf into main Sep 30, 2024
21 checks passed

andrescrz deleted the OPIK-99-sdk-implement-batching-for-spans-creation branch September 30, 2024 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OPIK-99] Implement batching for spans creation #298

[OPIK-99] Implement batching for spans creation #298

alexkuzmik commented Sep 20, 2024 •

edited

Loading

andrescrz left a comment

andrescrz left a comment

andrescrz left a comment

andrescrz Sep 25, 2024

alexkuzmik Sep 25, 2024

thiagohora Sep 25, 2024 •

edited

Loading

thiagohora Sep 25, 2024

andrescrz Sep 26, 2024

andrescrz Sep 25, 2024

alexkuzmik Sep 25, 2024

thiagohora Sep 25, 2024

thiagohora Sep 25, 2024

andrescrz Sep 26, 2024

andrescrz left a comment

andrescrz Sep 26, 2024

andrescrz Sep 26, 2024

This comment was marked as outdated.

andrescrz left a comment

[OPIK-99] Implement batching for spans creation #298

[OPIK-99] Implement batching for spans creation #298

Conversation

alexkuzmik commented Sep 20, 2024 • edited Loading

Details

Issues

Testing

Documentation

andrescrz left a comment

Choose a reason for hiding this comment

andrescrz left a comment

Choose a reason for hiding this comment

andrescrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagohora Sep 25, 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

andrescrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

andrescrz left a comment

Choose a reason for hiding this comment

alexkuzmik commented Sep 20, 2024 •

edited

Loading

thiagohora Sep 25, 2024 •

edited

Loading