diff --git a/e2e_test/bindings_go/test_go_oai_server.py b/e2e_test/bindings_go/test_go_oai_server.py index c44af90400..c813cfbd39 100644 --- a/e2e_test/bindings_go/test_go_oai_server.py +++ b/e2e_test/bindings_go/test_go_oai_server.py @@ -103,17 +103,17 @@ def test_streaming_multiple_messages(self, go_openai_client, go_oai_server): @pytest.mark.engine("sglang") @pytest.mark.gpu(1) @pytest.mark.model("meta-llama/Llama-3.2-1B-Instruct") -@pytest.mark.xfail( - reason="Llama-3.2-1B-Instruct doesn't reliably support tool calling with tool_choice=required", - strict=False, # Allow tests to pass if model happens to work -) class TestGoOAIServerFunctionCalling: """Tests for function calling through Go OAI server. - Note: Function calling requires the model to support tool use. - The meta-llama/Llama-3.2-1B-Instruct model may not reliably support tool_choice='required'. - These tests are marked as xfail to allow CI to pass while still - testing the Go OAI server's tool calling proxy functionality. + Note: Function calling requires the model to support tool use, and + meta-llama/Llama-3.2-1B-Instruct may not reliably emit a tool call even + with tool_choice='required'. Only that model-dependent step is allowed to + xfail (via an imperative ``pytest.xfail`` when the model produces no tool + call). Structural assertions — response shape, streamed index/argument + accumulation, JSON validity of any tool call the server *does* parse, and + tool_choice='none' suppression — are unconditional: they verify the Go OAI + server's proxy logic, not the model. """ TOOLS = [ @@ -162,17 +162,31 @@ def test_function_calling_non_streaming(self, go_openai_client, go_oai_server): stream=False, ) + # Structural: response shape (unconditional). assert response.choices is not None + assert len(response.choices) > 0 tool_calls = response.choices[0].message.tool_calls - assert tool_calls is not None, "Expected tool calls" - assert len(tool_calls) > 0, "Expected at least one tool call" + # Model-dependent: whether the 1B model emits a tool call at all. + if not tool_calls: + pytest.xfail( + "Llama-3.2-1B-Instruct did not emit a tool call despite " + "tool_choice='required' (known model limitation)" + ) + # Structural: any parsed tool call must be well-formed (unconditional). tool_call = tool_calls[0] assert tool_call.function.name == "get_weather" args = json.loads(tool_call.function.arguments) - assert "location" in args + assert isinstance(args, dict) + + # Model-dependent: argument quality. + if "location" not in args: + pytest.xfail( + f"Llama-3.2-1B-Instruct produced schema-incomplete arguments {args} " + "(known model limitation)" + ) logger.info(f"Tool call: {tool_call.function.name}({args})") def test_function_calling_streaming(self, go_openai_client, go_oai_server): @@ -191,6 +205,7 @@ def test_function_calling_streaming(self, go_openai_client, go_oai_server): stream=True, ) + # Structural: the stream itself must be well-formed (unconditional). chunks = list(response_stream) assert len(chunks) > 0 @@ -213,18 +228,37 @@ def test_function_calling_streaming(self, go_openai_client, go_oai_server): if tc.function.arguments: tool_calls_by_index[idx]["arguments"] += tc.function.arguments - assert len(tool_calls_by_index) > 0, "Expected tool calls in stream" + # Model-dependent: whether the 1B model emits a tool call at all. + if not tool_calls_by_index: + pytest.xfail( + "Llama-3.2-1B-Instruct streamed no tool call despite " + "tool_choice='required' (known model limitation)" + ) - # Verify first tool call + # Structural: index math and argument accumulation (unconditional). + assert 0 in tool_calls_by_index, ( + f"Streamed tool call indices must start at 0, got {sorted(tool_calls_by_index)}" + ) first_tc = tool_calls_by_index[0] assert first_tc["name"] == "get_weather" args = json.loads(first_tc["arguments"]) - assert "location" in args + assert isinstance(args, dict) + + # Model-dependent: argument quality. + if "location" not in args: + pytest.xfail( + f"Llama-3.2-1B-Instruct produced schema-incomplete arguments {args} " + "(known model limitation)" + ) logger.info(f"Streamed tool call: {first_tc['name']}({args})") def test_function_calling_tool_choice_none(self, go_openai_client, go_oai_server): - """Test that tool_choice='none' prevents function calls.""" + """Test that tool_choice='none' prevents function calls. + + This is a server-side suppression contract, not a model capability: + a weak tool-caller cannot make it fail, so nothing here may xfail. + """ _, _, model_path = go_oai_server response = go_openai_client.chat.completions.create( @@ -631,14 +665,15 @@ def test_system_message_only(self, go_openai_client, go_oai_server): @pytest.mark.model("meta-llama/Llama-3.2-1B-Instruct") @pytest.mark.xfail( reason="Go OAI server does not currently support n > 1 (multiple choices)", - strict=False, + strict=True, # deterministic capability gap: XPASS must fail so the marker is removed when n>1 lands ) class TestGoOAIServerMultipleChoices: """Tests for n parameter (multiple choices). Note: The Go OAI server currently does not support generating multiple - choices (n > 1). These tests are marked as xfail to document the expected - behavior and will pass when support is added. + choices (n > 1) — a deterministic server capability gap, not model + flakiness — so these tests are strict xfails: when support is added they + will XPASS and fail CI, forcing removal of the marker. """ def test_n_parameter_non_streaming(self, go_openai_client, go_oai_server): diff --git a/e2e_test/chat_completions/test_function_calling.py b/e2e_test/chat_completions/test_function_calling.py index 35b9d37bdc..1b32e8fa5c 100644 --- a/e2e_test/chat_completions/test_function_calling.py +++ b/e2e_test/chat_completions/test_function_calling.py @@ -976,52 +976,194 @@ def _is_flaky_test(self, test_name): """Check if the current test is marked as flaky for this class.""" return test_name in self.FLAKY_TESTS + # Prompt that clearly needs the declared ``get_weather`` tool. + AUTO_TOOL_NEEDED_MESSAGES = [{"role": "user", "content": "What's the weather in Tokyo?"}] + # Prompt that clearly needs no tool at all. + AUTO_NO_TOOL_MESSAGES = [ + {"role": "user", "content": "What is 2+2? Answer with just the number."} + ] + + def _assert_auto_tool_calls_valid(self, tool_calls, tools): + """Every produced tool call must name a declared tool with JSON-dict args.""" + declared_names = {tool["function"]["name"] for tool in tools} + for tool_call in tool_calls: + assert tool_call.function.name in declared_names, ( + f"Tool call names undeclared function {tool_call.function.name!r}; " + f"declared: {sorted(declared_names)}" + ) + args = json.loads(tool_call.function.arguments) + assert isinstance(args, dict), f"Arguments should parse to a dict, got {type(args)}" + def test_tool_choice_auto_non_streaming(self, model, api_client): - """Test tool_choice='auto' in non-streaming mode.""" + """Test tool_choice='auto' semantics in non-streaming mode. + + - A prompt that clearly needs the declared weather tool should yield a + tool call for a declared tool, with JSON args mentioning the location. + - A prompt that clearly needs no tool should yield non-empty text, + no tool calls, and finish_reason == 'stop'. + + For models registered in FLAKY_TESTS (weak tool-callers), the + *decision* (call vs. answer) is relaxed but every produced tool call + must still be structurally valid. + """ tools = get_test_tools() - messages = get_test_messages() + flaky = self._is_flaky_test("test_tool_choice_auto_non_streaming") + # --- Scenario 1: prompt that needs the weather tool --- response = api_client.chat.completions.create( model=model, - messages=messages, + messages=self.AUTO_TOOL_NEEDED_MESSAGES, max_tokens=2048, + temperature=0.2, tools=tools, tool_choice="auto", stream=False, ) - assert response.choices[0].message is not None - # With auto, tool calls are optional - - def test_tool_choice_auto_streaming(self, model, api_client): - """Test tool_choice='auto' in streaming mode.""" - - tools = get_test_tools() - messages = get_test_messages() + choice = response.choices[0] + tool_calls = choice.message.tool_calls + if tool_calls: + self._assert_auto_tool_calls_valid(tool_calls, tools) + assert any("tokyo" in (tc.function.arguments or "").lower() for tc in tool_calls), ( + f"Expected the location (Tokyo) in tool call args, got: {tool_calls}" + ) + assert choice.finish_reason == "tool_calls", ( + f"Expected finish_reason 'tool_calls', got {choice.finish_reason!r}" + ) + else: + assert flaky, ( + "tool_choice='auto' produced no tool call for a prompt that " + "clearly needs the declared weather tool" + ) + # Weak model answered directly; it must at least answer with text. + assert choice.message.content and choice.message.content.strip(), ( + "Expected non-empty text content when no tool call was made" + ) + assert choice.finish_reason == "stop", ( + f"Expected finish_reason 'stop', got {choice.finish_reason!r}" + ) + # --- Scenario 2: prompt that needs no tool --- response = api_client.chat.completions.create( model=model, - messages=messages, - max_tokens=2048, + messages=self.AUTO_NO_TOOL_MESSAGES, + max_tokens=256, + temperature=0.2, tools=tools, tool_choice="auto", - stream=True, + stream=False, ) - # Collect streaming response - content_chunks = [] - tool_call_chunks = [] + choice = response.choices[0] + if flaky and choice.message.tool_calls: + # Weak model spuriously called a tool; it must still be declared/valid. + self._assert_auto_tool_calls_valid(choice.message.tool_calls, tools) + assert choice.finish_reason == "tool_calls", ( + f"Expected finish_reason 'tool_calls', got {choice.finish_reason!r}" + ) + else: + assert not choice.message.tool_calls, ( + f"Expected no tool calls for a trivial arithmetic prompt, " + f"got: {choice.message.tool_calls}" + ) + assert choice.message.content and choice.message.content.strip(), ( + "Expected non-empty text content for a trivial arithmetic prompt" + ) + assert choice.finish_reason == "stop", ( + f"Expected finish_reason 'stop', got {choice.finish_reason!r}" + ) - for chunk in response: - if chunk.choices[0].delta.content: - content_chunks.append(chunk.choices[0].delta.content) - elif chunk.choices[0].delta.tool_calls: - tool_call_chunks.extend(chunk.choices[0].delta.tool_calls) + def test_tool_choice_auto_streaming(self, model, api_client): + """Test tool_choice='auto' semantics in streaming mode. - # Should complete without errors - assert isinstance(content_chunks, list) - assert isinstance(tool_call_chunks, list) + Same contract as the non-streaming variant, verified over accumulated + stream deltas (chunk collection mirrors + test_required_streaming_arguments_chunks_json). + """ + + tools = get_test_tools() + flaky = self._is_flaky_test("test_tool_choice_auto_streaming") + + def collect_stream(messages, max_tokens): + response = api_client.chat.completions.create( + model=model, + messages=messages, + max_tokens=max_tokens, + temperature=0.2, + tools=tools, + tool_choice="auto", + stream=True, + ) + content_parts = [] + tool_calls_by_index = {} + finish_reason = None + for chunk in response: + choice = chunk.choices[0] + if choice.finish_reason: + finish_reason = choice.finish_reason + if choice.delta.content: + content_parts.append(choice.delta.content) + if choice.delta.tool_calls: + for tc_delta in choice.delta.tool_calls: + tool_call = tool_calls_by_index.setdefault( + tc_delta.index, {"name": "", "arguments": ""} + ) + if tc_delta.function: + if tc_delta.function.name: + tool_call["name"] = tc_delta.function.name + if tc_delta.function.arguments: + tool_call["arguments"] += tc_delta.function.arguments + return "".join(content_parts), list(tool_calls_by_index.values()), finish_reason + + declared_names = {tool["function"]["name"] for tool in tools} + + def assert_streamed_calls_valid(tool_calls): + for tool_call in tool_calls: + assert tool_call["name"] in declared_names, ( + f"Tool call names undeclared function {tool_call['name']!r}" + ) + args = json.loads(tool_call["arguments"]) + assert isinstance(args, dict) + + # --- Scenario 1: prompt that needs the weather tool --- + content, tool_calls, finish_reason = collect_stream( + self.AUTO_TOOL_NEEDED_MESSAGES, max_tokens=2048 + ) + if tool_calls: + assert_streamed_calls_valid(tool_calls) + assert any("tokyo" in tc["arguments"].lower() for tc in tool_calls), ( + f"Expected the location (Tokyo) in streamed tool call args, got: {tool_calls}" + ) + assert finish_reason == "tool_calls", ( + f"Expected finish_reason 'tool_calls', got {finish_reason!r}" + ) + else: + assert flaky, ( + "tool_choice='auto' streamed no tool call for a prompt that " + "clearly needs the declared weather tool" + ) + assert content.strip(), "Expected non-empty streamed text when no tool call was made" + assert finish_reason == "stop", f"Expected finish_reason 'stop', got {finish_reason!r}" + + # --- Scenario 2: prompt that needs no tool --- + content, tool_calls, finish_reason = collect_stream( + self.AUTO_NO_TOOL_MESSAGES, max_tokens=256 + ) + if flaky and tool_calls: + assert_streamed_calls_valid(tool_calls) + assert finish_reason == "tool_calls", ( + f"Expected finish_reason 'tool_calls', got {finish_reason!r}" + ) + else: + assert not tool_calls, ( + f"Expected no streamed tool calls for a trivial arithmetic prompt, " + f"got: {tool_calls}" + ) + assert content.strip(), ( + "Expected non-empty streamed text for a trivial arithmetic prompt" + ) + assert finish_reason == "stop", f"Expected finish_reason 'stop', got {finish_reason!r}" def test_tool_choice_required_non_streaming(self, model, api_client): """Test tool_choice='required' in non-streaming mode.""" @@ -1508,10 +1650,13 @@ def test_conflicting_defs_required_tool_choice(self, model, api_client): class TestToolChoiceLlama(_TestToolChoiceBase): """Tests for tool_choice functionality with Llama model.""" - # Mark flaky tests for this model + # Mark flaky tests for this model — Llama-3.2-1B does not reliably + # decide correctly with tool_choice='auto'. FLAKY_TESTS = { "test_multi_tool_scenario_auto", "test_multi_tool_scenario_required", + "test_tool_choice_auto_non_streaming", + "test_tool_choice_auto_streaming", } @@ -1547,10 +1692,13 @@ class TestToolChoiceQwen(_TestToolChoiceBase): class TestToolChoiceMistral(_TestToolChoiceBase): """Tests for tool_choice functionality with Mistral model.""" - # Mark flaky tests for this model + # Mark flaky tests for this model — Mistral-7B-v0.3 is not a reliable + # tool_choice='auto' decision-maker. FLAKY_TESTS = { "test_multi_tool_scenario_auto", "test_multi_tool_scenario_required", + "test_tool_choice_auto_non_streaming", + "test_tool_choice_auto_streaming", } def test_complex_parameters_required_non_streaming(self, model, api_client): diff --git a/e2e_test/embeddings/test_basic.py b/e2e_test/embeddings/test_basic.py index 14e9391354..9d0a6f6a99 100644 --- a/e2e_test/embeddings/test_basic.py +++ b/e2e_test/embeddings/test_basic.py @@ -13,7 +13,9 @@ import logging +import openai import pytest +import smg_client logger = logging.getLogger(__name__) @@ -108,21 +110,43 @@ def test_embedding_dimensions_consistent(self, model, api_client): def test_embedding_empty_string(self, model, api_client): """Test embedding with empty string input. - Some models may handle empty strings differently. - This test verifies the API doesn't crash on empty input. + Contract: an empty-string input must either be embedded successfully + (one embedding with the model's usual dimension) or be rejected with a + 4xx client error. Anything else — a 5xx, a transport error, a malformed + success body — is a bug and must fail the test. + + Note: the test has swallowed both outcomes since its introduction + (see #812/#834 refactors), so the behavior current engines exhibit was + never recorded; the GPU lanes pin it down via this two-branch assert. """ + # Probe the model's embedding dimension with a known-good input. + probe = api_client.embeddings.create(model=model, input="dimension probe") + expected_dim = len(probe.data[0].embedding) + assert expected_dim > 0 + try: response = api_client.embeddings.create( model=model, input="", ) - # If it succeeds, verify structure - assert len(response.data) >= 1 - logger.info("Empty string embedding succeeded") - except Exception as e: - # Some models may reject empty strings - that's acceptable - logger.info("Empty string embedding rejected: %s", e) + except (openai.APIStatusError, smg_client.ApiError) as e: + # Rejection is acceptable only as a client error (4xx). + assert 400 <= e.status_code < 500, ( + f"Empty string input must be rejected with a 4xx client error, " + f"got HTTP {e.status_code}: {e}" + ) + logger.info("Empty string embedding rejected with HTTP %d", e.status_code) + else: + # Acceptance must produce exactly one well-formed embedding. + assert len(response.data) == 1, ( + f"Expected exactly 1 embedding for empty string, got {len(response.data)}" + ) + assert len(response.data[0].embedding) == expected_dim, ( + f"Empty string embedding dimension {len(response.data[0].embedding)} " + f"differs from model dimension {expected_dim}" + ) + logger.info("Empty string embedding succeeded (%d dims)", expected_dim) def test_embedding_unicode(self, model, api_client): """Test embedding with unicode characters. diff --git a/e2e_test/infra/gateway.py b/e2e_test/infra/gateway.py index 3e33bc6d38..d7c18481bc 100644 --- a/e2e_test/infra/gateway.py +++ b/e2e_test/infra/gateway.py @@ -412,15 +412,25 @@ def _worker_from_api_response(self, w: dict) -> WorkerInfo: }, ) - def list_workers(self, timeout: float = 5.0) -> list[WorkerInfo]: - """List all workers connected to the gateway.""" + def list_workers(self, timeout: float = 5.0, strict: bool = False) -> list[WorkerInfo]: + """List all workers connected to the gateway. + + With ``strict=True``, request failures and non-200 responses raise + instead of degrading to ``[]`` — required when an empty list is the + assertion target (e.g. "worker was removed"), where a swallowed error + would pass vacuously. + """ try: resp = httpx.get(f"{self.base_url}/workers", timeout=timeout) if resp.status_code == 200: data = resp.json() return [self._worker_from_api_response(w) for w in data.get("workers", [])] + if strict: + raise RuntimeError(f"GET /workers returned {resp.status_code}: {resp.text}") return [] except (httpx.RequestError, httpx.TimeoutException): + if strict: + raise return [] def add_worker( @@ -477,8 +487,11 @@ def remove_worker(self, worker_url: str, timeout: float = 10.0) -> tuple[bool, s f"{self.base_url}/workers/{worker_id}", timeout=timeout, ) - if resp.status_code == 200: - return True, "Worker removed" + # 200 = removed synchronously; 202 = removal accepted and queued + # for background processing. Either means the request succeeded — + # callers that need completion poll list_workers for absence. + if resp.status_code in (200, 202): + return True, resp.text return False, resp.text except (httpx.RequestError, httpx.TimeoutException) as e: return False, str(e) diff --git a/e2e_test/messages/test_tool_use.py b/e2e_test/messages/test_tool_use.py index 48fc26e1a4..d2ef14ab60 100644 --- a/e2e_test/messages/test_tool_use.py +++ b/e2e_test/messages/test_tool_use.py @@ -148,16 +148,22 @@ def test_tool_use_streaming(self, model, api_client): event_types.add(event.type) if event.type == "content_block_delta" and hasattr(event.delta, "partial_json"): input_json_deltas.append(event.delta.partial_json) + final_message = stream.get_final_message() assert "content_block_start" in event_types assert "content_block_delta" in event_types assert "content_block_stop" in event_types - # Concatenated partial_json should form valid JSON - if input_json_deltas: - full_json_str = "".join(input_json_deltas) - parsed = json.loads(full_json_str) - assert isinstance(parsed, dict) + # The weather prompt must produce a tool call (mirrors + # test_single_tool_call), so input_json deltas must be present: + # an empty list means the stream lost the tool-input deltas. + assert final_message.stop_reason == "tool_use" + assert len(input_json_deltas) > 0, "Expected input_json_delta events for the tool call" + + # Concatenated partial_json must form a valid JSON object + full_json_str = "".join(input_json_deltas) + parsed = json.loads(full_json_str) + assert isinstance(parsed, dict) def test_multiple_tools_available(self, model, api_client): """Test that model selects the correct tool when multiple are available.""" diff --git a/e2e_test/responses/test_tools_call.py b/e2e_test/responses/test_tools_call.py index cb11b797fb..27b790c107 100644 --- a/e2e_test/responses/test_tools_call.py +++ b/e2e_test/responses/test_tools_call.py @@ -438,7 +438,12 @@ def test_basic_function_call(self, model, api_client): assert message.content is not None text_parts = [part.text for part in message.content if part.type == "output_text"] full_text = " ".join(text_parts).lower() - assert "baby otter" in full_text or "aquarius" in full_text + # "baby otter" is the sentinel injected via function_call_output above; + # its presence proves the tool result was actually consumed. ("aquarius" + # would be satisfiable from the prompt alone.) + assert "baby otter" in full_text, ( + f"Tool-output sentinel 'baby otter' missing from final response: {full_text!r}" + ) def test_mcp_basic_tool_call(self, model, api_client): """Test basic MCP tool call (non-streaming).""" diff --git a/e2e_test/router/test_worker_api.py b/e2e_test/router/test_worker_api.py index dd50064619..3f0a6c0b2e 100644 --- a/e2e_test/router/test_worker_api.py +++ b/e2e_test/router/test_worker_api.py @@ -152,14 +152,29 @@ def test_igw_add_and_remove_worker(self): initial_count = len(gateway.list_workers()) logger.info("Worker count after add: %d", initial_count) - # Remove worker + # Remove worker — removal must succeed and the worker must + # disappear from the registry. success, msg = gateway.remove_worker(http_worker.base_url) - if success: - logger.info("Removed worker: %s", msg) - final_count = len(gateway.list_workers()) - logger.info("Worker count after remove: %d", final_count) - else: - logger.warning("Remove worker not supported: %s", msg) + assert success, f"Failed to remove worker: {msg}" + logger.info("Removed worker: %s", msg) + + # Poll briefly in case removal is applied asynchronously. + # Sample once up front so the assertion below always has a + # real observation to report, even if the deadline has already + # passed by the time the loop condition is first evaluated. + deadline = time.perf_counter() + 15 + remaining_urls = [w.url for w in gateway.list_workers()] + while http_worker.base_url in remaining_urls and time.perf_counter() < deadline: + time.sleep(1.0) + remaining_urls = [w.url for w in gateway.list_workers()] + + # Authoritative final read: strict, so a failed /workers call + # cannot masquerade as "no workers left". + remaining_urls = [w.url for w in gateway.list_workers(strict=True)] + assert http_worker.base_url not in remaining_urls, ( + f"Worker {http_worker.base_url} still listed after removal: {remaining_urls}" + ) + logger.info("Worker count after remove: %d", len(remaining_urls)) finally: gateway.shutdown() finally: