chore(weave): Split out Trace and Query tests; move trace tests into dedicated tox envs across py39-312 #2403

andrewtruong · 2024-09-17T15:07:41Z

Description

https://wandb.atlassian.net/browse/WB-20964

This PR splits out tests into dedicated tox envs for {trace, trace_server, and first party integrations} across the python versions we support.

Py312 tests are legitimately failing, but are commented out to be addressed in a follow-up PR because those changes would otherwise be noisy here. The issues seem to stem from a behaviour change in runtime_checkable which breaks isinstance(obj, Op) checks:
https://docs.python.org/3/library/typing.html#typing.runtime_checkable
This PR requires the companion:
https://github.com/wandb/core/pull/23983

Testing

this is testing :)

circle-job-mirror · 2024-09-17T15:09:22Z

Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=4da007ea05eb11473835fa24c0d91e5ee3b529db

This reverts commit 0dda52b.

.github/workflows/test.yaml

tssweeney · 2024-09-18T20:43:13Z

.github/workflows/test.yaml

@@ -133,7 +220,7 @@ jobs:
          --timeout=90 \
          --ddtrace \
          --durations=5 \
-          ./integrations ./legacy ./trace_server ./trace ./tests
+          ./legacy


This should be removed. Anything depending on the client fixture in legacy is misplaced. I just verified this by looking at the 3 cases

ok, I found 4 cases and commented them out for now.

why not bring them over into trace? They were misplaced?

I'll bring test_weave_finish_unsets_client over.

I don't think the Monitoring tests are valuable anymore. We don't have that path, and the OpenAI and basic logging stuff has its own set of tests already.

.github/workflows/test.yaml

tssweeney · 2024-09-18T20:45:08Z

requirements.legacy.txt

@@ -0,0 +1,61 @@
+# These are the base Weave requirements, enough for weave tracking and evaluation


This can be severely trimmed down - much of this is trace specific. fine to be a followup

Is the minimal set described here? If yes we can replace:
https://github.com/wandb/core/blob/master/services/weave-python/requirements.txt

tssweeney · 2024-09-18T20:46:09Z

requirements.txt

@@ -1,5 +1,10 @@
 # These are the base Weave requirements, enough for weave tracking and evaluation
 # to work.
+# -r requirements.legacy.txt
+# COPIED FROM LEGACY:


why not put this comment next to where you added these reqs?

This is a temporary "inlining" of -r requirements.legacy.txt because the -r ... syntax is not valid for pip installs using pyproject.toml. I think this form is more obvious that it's a copy-paste vs. annotating individual reqs

In future, we'll remove this altogether and just use pyproject.toml

tssweeney · 2024-09-18T20:48:04Z

.github/workflows/test.yaml

+      fail-fast: false
+    services:
+      wandbservice:
+        image: us-central1-docker.pkg.dev/wandb-production/images/local-testcontainer:master


why this base?

That's the container we use to test things that need gorilla.

Do you think there's a better container to use?

tssweeney · 2024-09-18T20:49:47Z

weave/docker/Dockerfile.python-client.test

@@ -10,6 +10,6 @@ RUN python3 -m venv venv
 RUN --mount=type=cache,target=/root/.cache /bin/bash -c \
    "source venv/bin/activate && \
    pip install --upgrade pip && \
-    pip install -r requirements.test.txt"
+    pip install -r requirements.legacy.test.txt"


This doesn't seem right - the legacy tests should be using weave/legacy/Dockerfile.test. This Dockerfile would be used for your tox tests (or deleted if we want to use wandbs)

I'm not very familiar here, but I'm following this part of the tests:
https://github.com/wandb/weave/blame/andrew/tox3/.github/workflows/test.yaml#L37-L40

Which seems to be:

A unit test image which is used for the legacy tests below

An integration test image which is used for the weave tests in core

tssweeney · 2024-09-18T20:50:26Z

weave/legacy/tests/test_wb_data_types.py

@@ -535,7 +535,7 @@ def test_annotated_images_in_tables(fake_wandb):

 def test_annotated_legacy_images_in_tables(fake_wandb):
    # Mocking this property makes the payload look like the legacy version.
-    from wandb.data_types import _ImageFileType
+    from wandb.sdk.data_types.image import _ImageFileType


this is fixed in master

andrewtruong requested a review from a team as a code owner September 17, 2024 15:07

andrewtruong marked this pull request as draft September 17, 2024 15:07

andrewtruong force-pushed the andrew/tox3 branch 3 times, most recently from ca5902d to 95217aa Compare September 17, 2024 15:32

test

4e15452

andrewtruong force-pushed the andrew/tox3 branch from 95217aa to 4e15452 Compare September 17, 2024 15:36

test

de289a1

andrewtruong force-pushed the andrew/tox3 branch from b562ec5 to de289a1 Compare September 17, 2024 15:52

andrewtruong added 20 commits September 17, 2024 12:07

test

6fcdc91

test

1f68dc6

test

83c1332

test

ad4b0dd

test

586996f

test

c468cd3

test

fa7f5b4

test

8ea9fda

test

ec6768f

test

23e2a31

test

3221153

test

9bcbaea

test2

0dda52b

Revert "test2"

36eb314

This reverts commit 0dda52b.

test

e2cd66a

test

2c52844

test

59939f4

test

370318e

test

d19c102

test

f356c08

andrewtruong added 7 commits September 17, 2024 21:14

test2

cf0f0af

test

3423d6e

test

e9677a9

test

c55c793

test

a314b51

test

3d14563

test

b204c9d

andrewtruong changed the title ~~test4~~ chore(weave): Split out tests into dedicated tox envs across multiple python versions Sep 18, 2024

andrewtruong changed the title ~~chore(weave): Split out tests into dedicated tox envs across multiple python versions~~ chore(weave): Split out Trace and Query tests into dedicated tox envs across py39-312 Sep 18, 2024

andrewtruong added 3 commits September 18, 2024 09:04

test

7a60870

test

2dede3c

test

3888bd1

andrewtruong marked this pull request as ready for review September 18, 2024 15:18

andrewtruong changed the title ~~chore(weave): Split out Trace and Query tests into dedicated tox envs across py39-312~~ chore(weave): Split out Trace and Query tests; move trace tests into dedicated tox envs across py39-312 Sep 18, 2024

tssweeney reviewed Sep 18, 2024

View reviewed changes

.github/workflows/test.yaml Outdated Show resolved Hide resolved

tssweeney reviewed Sep 18, 2024

View reviewed changes

.github/workflows/test.yaml Outdated Show resolved Hide resolved

tssweeney reviewed Sep 18, 2024

View reviewed changes

.github/workflows/test.yaml Show resolved Hide resolved

tssweeney reviewed Sep 18, 2024

View reviewed changes

andrewtruong added 3 commits September 18, 2024 21:31

test

5ebf0a0

test

2fdd643

test??

7a52f88

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(weave): Split out Trace and Query tests; move trace tests into dedicated tox envs across py39-312 #2403

chore(weave): Split out Trace and Query tests; move trace tests into dedicated tox envs across py39-312 #2403

andrewtruong commented Sep 17, 2024 •

edited

Loading

circle-job-mirror bot commented Sep 17, 2024 •

edited

Loading

tssweeney Sep 18, 2024

andrewtruong Sep 19, 2024

tssweeney Sep 19, 2024

andrewtruong Sep 19, 2024

tssweeney Sep 18, 2024

andrewtruong Sep 19, 2024

tssweeney Sep 18, 2024

andrewtruong Sep 19, 2024

tssweeney Sep 18, 2024

andrewtruong Sep 19, 2024

tssweeney Sep 18, 2024

andrewtruong Sep 19, 2024

tssweeney Sep 18, 2024

		@@ -0,0 +1,61 @@
		# These are the base Weave requirements, enough for weave tracking and evaluation

chore(weave): Split out Trace and Query tests; move trace tests into dedicated tox envs across py39-312 #2403

Are you sure you want to change the base?

chore(weave): Split out Trace and Query tests; move trace tests into dedicated tox envs across py39-312 #2403

Conversation

andrewtruong commented Sep 17, 2024 • edited Loading

Description

Testing

circle-job-mirror bot commented Sep 17, 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

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

andrewtruong commented Sep 17, 2024 •

edited

Loading

circle-job-mirror bot commented Sep 17, 2024 •

edited

Loading