-
Notifications
You must be signed in to change notification settings - Fork 1
fix/feat: P0 bug fixes, type annotations, docstrings, and more tests #7
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,8 +100,22 @@ def get_model_name_from_image_path(image_path: Path | str) -> str: | |
| return stem.split("_")[-1] | ||
|
|
||
|
|
||
| def split_senmantic_and_data_in_t2i_prompt(t2i_prompt: str) -> tuple[str, str]: | ||
| """ | ||
| Split the semantic and data parts of the T2I prompt. | ||
| def split_semantic_and_data_in_t2i_prompt(t2i_prompt: str) -> tuple[str, str]: | ||
| """Split a T2I prompt into semantic description and data sections. | ||
|
|
||
| Expects the prompt to contain the separator "The given data is:". | ||
| Returns a (semantic_part, data_part) tuple. | ||
|
|
||
| Raises: | ||
| ValueError: If the separator is not found in the prompt. | ||
| """ | ||
| return t2i_prompt.split("The given data is:") | ||
| if not t2i_prompt: | ||
| raise ValueError("Prompt cannot be empty or None.") | ||
| separator = "The given data is:" | ||
| if separator not in t2i_prompt: | ||
|
Comment on lines
+114
to
+115
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If if not t2i_prompt:
raise ValueError("Prompt cannot be empty or None.")
separator = "The given data is:"
if separator not in t2i_prompt: |
||
| raise ValueError( | ||
| f"Prompt does not contain expected separator '{separator}'. " | ||
| "Cannot split semantic and data sections." | ||
| ) | ||
| parts = t2i_prompt.split(separator, maxsplit=1) | ||
| return parts[0], parts[1] | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,17 +21,38 @@ def _get_caller(self, provider: str) -> LLMCaller: | |||||||||||||||||||||
| def call_text_generation( | ||||||||||||||||||||||
| self, model: str, prompt: str, **kwargs: Any | ||||||||||||||||||||||
| ) -> Union[dict, str]: | ||||||||||||||||||||||
| """Generate text and parse any fenced code block / JSON in the response. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||
| Parsed dict if the response contains valid JSON, otherwise a plain string. | ||||||||||||||||||||||
| Returns an empty string if the provider returns None or an empty response. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| text_response = self._caller.generate_text(model, prompt, **kwargs) | ||||||||||||||||||||||
| if not text_response: | ||||||||||||||||||||||
| return "" | ||||||||||||||||||||||
| return extract_from_markdown(text_response) | ||||||||||||||||||||||
|
Comment on lines
30
to
33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the LLM provider returns an empty response or text_response = self._caller.generate_text(model, prompt, **kwargs)
if not text_response:
return ""
return extract_from_markdown(text_response) |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def call_image_understanding( | ||||||||||||||||||||||
| self, model: str, prompt: str, image_path: str, **kwargs: Any | ||||||||||||||||||||||
| ) -> Union[dict, str]: | ||||||||||||||||||||||
| """Analyse an image and parse any fenced code block / JSON in the response. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||
| Parsed dict if the response contains valid JSON, otherwise a plain string. | ||||||||||||||||||||||
| Returns an empty string if the provider returns None or an empty response. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| text_response = self._caller.understand_image( | ||||||||||||||||||||||
| model, prompt, image_path, **kwargs | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| if not text_response: | ||||||||||||||||||||||
| return "" | ||||||||||||||||||||||
| return extract_from_markdown(text_response) | ||||||||||||||||||||||
|
Comment on lines
44
to
49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to
Suggested change
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def call_image_generation(self, model: str, prompt: str, **kwargs: Any) -> PILImage: | ||||||||||||||||||||||
| """Generate an image and return it as a PIL Image. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||
| PIL Image object of the generated image. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| pil_image = self._caller.generate_image(model, prompt, **kwargs) | ||||||||||||||||||||||
| return pil_image | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| """Unit tests for the @register_caller decorator and CALLER_REGISTRY.""" | ||
|
|
||
| import pytest | ||
|
|
||
| from igenbench.utils.llm.caller_registry import CALLER_REGISTRY, register_caller | ||
| from igenbench.utils.llm.llm_caller import LLMCaller | ||
|
|
||
|
|
||
| def test_register_caller_adds_to_registry(): | ||
| name = "_test_provider_add" | ||
| try: | ||
|
|
||
| @register_caller(name) | ||
| class _DummyCaller(LLMCaller): | ||
| pass | ||
|
|
||
| assert name in CALLER_REGISTRY | ||
| assert CALLER_REGISTRY[name] is _DummyCaller | ||
| finally: | ||
| CALLER_REGISTRY.pop(name, None) | ||
|
|
||
|
|
||
| def test_register_caller_duplicate_raises(): | ||
| name = "_test_provider_dup" | ||
| try: | ||
|
|
||
| @register_caller(name) | ||
| class _First(LLMCaller): | ||
| pass | ||
|
|
||
| with pytest.raises(ValueError, match="already registered"): | ||
|
|
||
| @register_caller(name) | ||
| class _Second(LLMCaller): | ||
| pass | ||
|
|
||
| finally: | ||
| CALLER_REGISTRY.pop(name, None) | ||
|
|
||
|
|
||
| def test_register_caller_returns_class_unchanged(): | ||
| name = "_test_provider_ret" | ||
| try: | ||
|
|
||
| @register_caller(name) | ||
| class _MyProvider(LLMCaller): | ||
| pass | ||
|
|
||
| assert _MyProvider.__name__ == "_MyProvider" | ||
| finally: | ||
| CALLER_REGISTRY.pop(name, None) | ||
|
|
||
|
|
||
| def test_built_in_providers_registered(): | ||
| for provider in ("google", "openrouter", "replicate"): | ||
| assert provider in CALLER_REGISTRY, f"{provider} not found in registry" |
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.
If
responseisNone(e.g., if the API call failed or returned an empty response), callingstr(response)will setanalysisto the literal string"None". It is better to default to an empty string""to avoid saving"None"as the analysis result.