Repository navigation
feat: Integrations core classes - #24
Conversation
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
psschwei
left a comment
There was a problem hiding this comment.
Couple of comments here:
-
We should confirm with @nrfulton where we should put the integrations. My understanding is that mellea-contribs is going to be a single package, but let's confirm. (if that is the case, we'll need to combine the pyproject.tomls)
-
10 of the tests failed when I tried running them. Here's how I ran them:
cd integrations/mellea-integration-core
uv sync
uv add pytest
uv run pytestand the failing tests:
FAILED tests/test_base.py::test_prepare_generation_no_messages - Failed: DID NOT RAISE <class 'ValueError'>
FAILED tests/test_base.py::test_agenerate_with_mellea_chat - Failed: async def functions are not natively supported.
FAILED tests/test_base.py::test_agenerate_with_mellea_instruct - Failed: async def functions are not natively supported.
FAILED tests/test_base.py::test_full_agenerate_flow - Failed: async def functions are not natively supported.
FAILED tests/test_message_converter.py::test_extract_content_from_response_fallback - AttributeError: Mock object has no attribute '__str__'
FAILED tests/test_tool_converter.py::test_extract_tool_schema_basic - AssertionError: assert <Mock name='test_tool.name' id='4418542224'> == 'test_tool'
FAILED tests/test_tool_converter.py::test_get_tool_callable_not_found - Failed: DID NOT RAISE <class 'ValueError'>
FAILED tests/test_tool_converter.py::test_extract_tool_calls_from_response_tool_calls_attr - AssertionError: assert <Mock name='test_tool.name' id='4418765072'> == 'test_tool'
FAILED tests/test_tool_converter.py::test_extract_tool_calls_from_response_private_attr - AssertionError: assert <Mock name='test_tool.name' id='4418769968'> == 'test_tool'
FAILED tests/test_tool_converter.py::test_parse_tool_calls_default_implementation - AssertionError: assert <Mock name='test_tool.name' id='4418772944'> == 'test_tool'
- related point, we should add these tests to the CI
|
I might forget check these tests. Thanks for checking. |
|
Tests can be added once we can confirm home of these integrations. |
| - ✅ **Tool Conversion**: Utilities for handling tool calling across frameworks | ||
| - ✅ **Requirements Support**: Built-in support for Mellea's validation features | ||
| - ✅ **Async Support**: Both sync and async generation patterns | ||
| - ✅ **Type Safety**: Protocol-based design with type hints |
There was a problem hiding this comment.
While we use type hints, a lot of them are Any which isn't exactly type safe
There was a problem hiding this comment.
Adding some clarification here.
| valid_roles = ["system", "user", "assistant", "tool"] | ||
| if role not in valid_roles: | ||
| role = "user" # Default fallback |
There was a problem hiding this comment.
should we instead log an error, since if the role isn't valid that may mean we have a bug in the conversion logic ?
| ValueError: If validation failed and no samples were generated | ||
| """ | ||
| # Check if this is a SamplingResult object | ||
| if hasattr(response, "success") and hasattr(response, "result"): |
There was a problem hiding this comment.
Would it be better to explicitly check, something like
| if hasattr(response, "success") and hasattr(response, "result"): | |
| if SamplingResult is not None and isinstance(response, SamplingResult): |
?
| else: | ||
| # If response has no content or value attribute, return empty string | ||
| # rather than string representation of the object | ||
| return "" |
There was a problem hiding this comment.
would it be worth logging this (assuming an empty response could be a sign of something having gone wrong)?
|
|
||
| # Pattern to match ToolCall objects in the string | ||
| # Matches: ToolCall(function=Function(name='tool_name', arguments={...})) | ||
| pattern = r"ToolCall\(function=Function\(name='([^']+)',\s*arguments=(\{[^}]+\})\)\)" |
There was a problem hiding this comment.
is there any chance that arguments could be a nested dict? e.g. {'a': {'b': 1}} if so, I think the regex may fail on that case
There was a problem hiding this comment.
Updated to accept the nested case. Comments are added for clarification.
| ## Existing Integrations | ||
|
|
||
| This core package is used by: | ||
|
|
||
| - **[mellea-langchain](../langchain/)**: LangChain integration | ||
| - **[mellea-crewai](../crewai/)**: CrewAI integration | ||
| - **[mellea-dspy](../dspy/)**: DSPy integration |
There was a problem hiding this comment.
we may want to hold off adding these until we land those integrations
| # Note: This pattern uses a greedy match to handle nested dictionaries | ||
| # It will match from the first { to the last } in the arguments section | ||
| pattern = r"ToolCall\(function=Function\(name='([^']+)',\s*arguments=(\{.+?\})\)\)" |
There was a problem hiding this comment.
technically, .+? is lazy not greedy. for greedy, you'd need just.+
it might be worth doing something like regex-extracting just the tool name, finding the arguments prefix, and then using a brace-counting scan (a small helper to match closing and opening braces) to grab the full dict string ? wdyt?
There was a problem hiding this comment.
Thanks for suggestion. I implemented it.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
psschwei
left a comment
There was a problem hiding this comment.
I think I'm generally ok with this, pending resolution of the location question.
@AngeloDanducci can you also give a review?
| try: | ||
| schema["function"]["parameters"] = tool.args_schema.schema() | ||
| except Exception: | ||
| # If schema extraction fails, use empty dict |
There was a problem hiding this comment.
May be worth logging the exception, doesn't seem like a big deal though.
There was a problem hiding this comment.
Had a couple of failures in here
FAILED tests/test_base.py::test_handle_sampling_results_success - AssertionError: assert <Mock id='4426200416'> == <Mock name='mock.result' id='4426200128'>
FAILED tests/test_base.py::test_handle_sampling_results_failure_with_samples - AssertionError: assert <Mock id='4426102064'> == <Mock id='4426097168'>
FAILED tests/test_base.py::test_handle_sampling_results_failure_no_samples - Failed: DID NOT RAISE <class 'ValueError'>
|
Couple of test failures but otherwise LGTM. Re location - I think it makes sense that each "sub project" could have its own toml but it's not a huge deal to me either way. |
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
Thanks for review. I made fixes and suggested changes. |
AngeloDanducci
left a comment
There was a problem hiding this comment.
Per offline discussion, we should likely move to uv workspaces for the approach of multi interconnected packages in the same repo.
If you want to set that up in this PR great if not feel free to merge and I'll open an issue to follow up after we get the integrations core PRs in.
Fix: generative-computing/mellea#447