chore(weave): Cleanup conftest #2580
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=da79488c7327a2295158187a99836054db953c7b |
| # Note: this test only works with the `trace_init_client` fixture | ||
| @pytest.mark.skip(reason="TODO: Skipping since it seems to rely on the testcontainer") | ||
| def test_ref_get_no_client(trace_init_client): | ||
| trace_client = trace_init_client.client | ||
| data = weave.publish(42) | ||
| data_got = weave.ref(data.uri()).get() | ||
| assert data_got == 42 | ||
|
|
||
| # clear the graph client effectively "de-initializing it" | ||
| with _no_graph_client(): | ||
| # This patching is required just to make the test path work | ||
| with _patched_default_initializer(trace_client): | ||
| # Now we will try to get the data again | ||
| data_got = weave.ref(data.uri()).get() | ||
| assert data_got == 42 | ||
|
|
||
|
|
There was a problem hiding this comment.
is this still needed?
There was a problem hiding this comment.
how long has this been skipped?
There was a problem hiding this comment.
This has only been skipped since the refactor, but it uses this trace_init_client that I don't see in other tests
There was a problem hiding this comment.
Maybe it can be rewritten with the normal client fixture
andrewtruong
changed the title
| @pytest.mark.skip("too slow") | ||
| def test_trace_call_query_filter_wb_run_ids(client, user_by_api_key_in_env): | ||
| call_spec = simple_line_call_bootstrap(init_wandb=True) | ||
|
|
||
| res = get_all_calls_asserting_finished(client, call_spec) | ||
|
|
||
| wb_run_ids = list(set([call.wb_run_id for call in res.calls]) - set([None])) | ||
|
|
||
| for wb_run_ids, exp_count in [ | ||
| # Test the None case | ||
| (None, call_spec.total_calls), | ||
| # Test the empty list case | ||
| ([], call_spec.total_calls), | ||
| # Test List (of 1) | ||
| (wb_run_ids, call_spec.run_calls), | ||
| ]: | ||
| inner_res = get_client_trace_server(client).calls_query( | ||
| tsi.CallsQueryReq( | ||
| project_id=get_client_project_id(client), | ||
| filter=tsi.CallsFilter(wb_run_ids=wb_run_ids), | ||
| ) | ||
| ) | ||
|
|
||
| assert len(inner_res.calls) == exp_count | ||
|
|
||
|
|
There was a problem hiding this comment.
do we need this?
| def pytest_sessionfinish(session, exitstatus): | ||
| if exitstatus == pytest.ExitCode.NO_TESTS_COLLECTED: | ||
| session.exitstatus = 0 | ||
| def pytest_addoption(parser): |
There was a problem hiding this comment.
nice, does this bring back the --weave-server=clickhouse in pytest.ini?
| @@ -233,7 +513,7 @@ def create_client(request) -> weave_init.InitializedClient: | |||
|
|
|||
|
|
|||
| @pytest.fixture() | |||
| def client(request) -> Generator[weave_client.WeaveClient, None, None]: | |||
| def client(request): | |||
There was a problem hiding this comment.
I don't think it's helpful in this context
| # Note: this test only works with the `trace_init_client` fixture | ||
| @pytest.mark.skip(reason="TODO: Skipping since it seems to rely on the testcontainer") | ||
| def test_ref_get_no_client(trace_init_client): | ||
| trace_client = trace_init_client.client | ||
| data = weave.publish(42) | ||
| data_got = weave.ref(data.uri()).get() | ||
| assert data_got == 42 | ||
|
|
||
| # clear the graph client effectively "de-initializing it" | ||
| with _no_graph_client(): | ||
| # This patching is required just to make the test path work | ||
| with _patched_default_initializer(trace_client): | ||
| # Now we will try to get the data again | ||
| data_got = weave.ref(data.uri()).get() | ||
| assert data_got == 42 | ||
|
|
||
|
|
There was a problem hiding this comment.
how long has this been skipped?
| @@ -119,46 +118,6 @@ def op_with_unknown_param() -> int: | |||
| assert op_with_unknown_param() == 12 | |||
|
|
|||
|
|
|||
| @pytest.mark.skip("artifact file download doesn't work here?") | |||
| def test_saveloop_idempotent_with_refs(user_by_api_key_in_env): | |||
There was a problem hiding this comment.
i feel like having these skipped tests aren't that bad, unless they are using old fixtures or otherwise dead code
There was a problem hiding this comment.
This uses the old weave.type() stuff which is no longer supported
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def strict_op_saving(enabled: bool) -> typing.Generator[bool, None, None]: |
There was a problem hiding this comment.
afaik you don't need this anymore
There was a problem hiding this comment.
Do you need this?
| @@ -1,406 +0,0 @@ | |||
| import contextlib | |||
There was a problem hiding this comment.
why do we no longer need this? Any chance we add this back in?
|
added some more notes |
Removes unnecessary tests, fixtures, etc. and consolidates conftest.
Notably:
trace_server_clickhouse_conftest.pyandwandb_system_tests_conftest.pyinto just the oneconftest.pyfilestrict_op_savingcontextvar and fixture (I believe this is not required anymore)