Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughRefactors adapter invocation flow: adds BaseAdapter._resolve_stream and abstract _invoke/_stream_call/_acall/_call, implements them in LocalAdapter and RayAdapter, fixes a LocalAdapter import typo, updates tests, CI, and lint config. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Base as BaseAdapter
participant Impl as AdapterImpl (Local/Ray)
participant Remote as RemoteFunc/DeploymentHandle
rect rgba(200,220,255,0.5)
Caller->>Base: call_func(api, **kwargs)
Base->>Base: _resolve_stream(api, kwargs)
alt stream = true
Base->>Impl: _stream_call(func, **kwargs)
Impl->>Remote: stream generator / remote streaming
Remote-->>Caller: stream data
else stream = false
Base->>Impl: _invoke(func, **kwargs)
Impl->>Impl: determine coroutine or handle
alt async / DeploymentHandle
Impl->>Impl: _acall(func/handle, **kwargs)
Impl->>Remote: await remote call
Remote-->>Caller: result
else sync
Impl->>Impl: _call(func, **kwargs)
Impl->>Remote: local call
Remote-->>Caller: result
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai gererate tests |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/framex/adapter/ray_adapter.py`:
- Around line 66-68: RayAdapter._call currently invokes func(**kwargs) which
fails for Ray DeploymentHandle method references; change RayAdapter._call to
call func.remote(**kwargs) (mirroring _acall) and return its result (or, if you
prefer to disallow sync calls, raise NotImplementedError with a clear message).
Update the implementation referenced by RayAdapter._call so it treats Ray
deployment method references like _acall does (using func.remote) and keep
get_handle_func/deployment_handle/DeploymentHandle logic unchanged.
🧹 Nitpick comments (2)
src/framex/adapter/base.py (1)
27-31: Side-effect onkwargsis subtle but intentional — consider a brief comment.
_resolve_streammutates the caller'skwargsdict in-place (adding"proxy_path"). This works becausecall_funcpasses the same dict to subsequent calls, but it's easy to miss during future maintenance. A one-line comment at line 29 would help.src/framex/adapter/local_adapter.py (1)
73-75:_stream_callrequires async iterables, but lacks type enforcement to prevent sync generators.In
base.pyline 40, the result of_stream_callis consumed viaasync for chunk in self._stream_call(...). This requires the return value to have an__aiter__method. While the framework's streaming functions are async generators (e.g.,echo_streamreturnsAsyncGenerator[str, None]), there is no type contract or runtime validation preventing someone from mistakenly using a sync generator as a streaming function, which would causeTypeError: 'async for' requires an object with __aiter__ method.Consider wrapping the result to support both sync and async generators:
Proposed defensive wrapper
`@override` - def _stream_call(self, func: Callable[..., Any], **kwargs: Any) -> Any: - return func(**kwargs) + async def _stream_call(self, func: Callable[..., Any], **kwargs: Any) -> Any: + result = func(**kwargs) + if hasattr(result, '__aiter__'): + async for chunk in result: + yield chunk + else: + for chunk in result: + yield chunk
Comment on lines
+66
to
+68
| @override | ||
| def _call(self, func: Callable[..., Any], **kwargs: Any) -> Any: | ||
| return func(**kwargs) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's explore the file structure
ls -la src/framex/adapter/
# Look at the ray_adapter.py file around the _call method
cat -n src/framex/adapter/ray_adapter.py | head -100Repository: touale/FrameX-kit
Length of output: 3356
🏁 Script executed:
#!/bin/bash
# Check what get_handle returns and how it's used
rg -n 'get_handle\b' src/framex/adapter/ray_adapter.py -A 5 -B 2Repository: touale/FrameX-kit
Length of output: 394
🏁 Script executed:
#!/bin/bash
# Check the _invoke method and routing logic
rg -n '_invoke|_acall|_call' src/framex/adapter/ray_adapter.py -B 2 -A 5Repository: touale/FrameX-kit
Length of output: 860
🏁 Script executed:
#!/bin/bash
# Look at base.py to understand get_handle_func
rg -n 'get_handle_func\|get_handle' src/framex/adapter/base.py -A 5 -B 2Repository: touale/FrameX-kit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check the base.py implementation
cat -n src/framex/adapter/base.pyRepository: touale/FrameX-kit
Length of output: 2984
🏁 Script executed:
#!/bin/bash
# Search for where _invoke is called from
rg -n '_invoke\|get_handle' src/framex/ -A 3 -B 2 --type pyRepository: touale/FrameX-kit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check local_adapter.py to see how it implements _call
cat -n src/framex/adapter/local_adapter.pyRepository: touale/FrameX-kit
Length of output: 3112
🏁 Script executed:
#!/antml/bash
# Search for all places where _invoke is called
rg -n 'await.*_invoke\|_invoke\(' src/framex/ -B 3 -A 2 --type pyRepository: touale/FrameX-kit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check for any sync function calls or patterns
rg -n '_call\(' src/framex/adapter/ -B 3 -A 2 --type pyRepository: touale/FrameX-kit
Length of output: 3419
🏁 Script executed:
#!/bin/bash
# Search for tests or usage of _invoke to understand what functions are passed
rg -n 'call_func\|_invoke' src/framex/ -B 5 -A 5 --type py | head -100Repository: touale/FrameX-kit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that exercise the _call path
find src/framex -name "*.py" -type f | xargs rg -l 'test.*_invoke\|test.*call_func' 2>/dev/nullRepository: touale/FrameX-kit
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look for documentation or comments about Ray DeploymentHandle behavior
rg -n 'DeploymentHandle\|method.*ref\|\.remote' src/framex/ --type pyRepository: touale/FrameX-kit
Length of output: 43
RayAdapter._call is reachable but broken for sync remote methods.
When get_handle_func calls getattr(deployment_handle, func_name) on a Ray DeploymentHandle, it returns a method reference—not another DeploymentHandle. For a sync (non-async) method on a Ray deployment:
isinstance(func, DeploymentHandle)→ Falseinspect.iscoroutinefunction(func)→ False- Falls through to
_call
However, the current _call implementation (return func(**kwargs)) is incorrect for Ray. Ray method references require .remote() to execute remotely (see _acall's func.remote(**kwargs)). Calling func(**kwargs) directly will fail at runtime because it attempts local execution instead of remote execution.
The _call path should either:
- Use
.remote()like_acalldoes, or - Raise an error if sync remote methods are not supported
🤖 Prompt for AI Agents
In `@src/framex/adapter/ray_adapter.py` around lines 66 - 68, RayAdapter._call
currently invokes func(**kwargs) which fails for Ray DeploymentHandle method
references; change RayAdapter._call to call func.remote(**kwargs) (mirroring
_acall) and return its result (or, if you prefer to disallow sync calls, raise
NotImplementedError with a clear message). Update the implementation referenced
by RayAdapter._call so it treats Ray deployment method references like _acall
does (using func.remote) and keep
get_handle_func/deployment_handle/DeploymentHandle logic unchanged.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 5850 🏁 Script executed: Length of output: 10560 🏁 Script executed: Length of output: 5365
Test Suite Plan for Adapter RefactoringI'll create a comprehensive test suite covering:
💡 Use |
Owner
Author
|
Tip For best results, initiate chat on the files or code changes.
I previously provided a comprehensive test plan for your adapter refactoring changes. Would you like me to:
Please let me know what you'd like me to do! 🐰 💡 Use |
Owner
Author
|
@coderabbitai generate test code |
|
Tip For best results, initiate chat on the files or code changes.
Test Suite Implementation Plan
💡 Use |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Copyable Unit Test EditsCopyable Editstests/adapter/init.pyThis is a new file. tests/adapter/conftest.pyThis is a new file. tests/adapter/test_base.pyThis is a new file. tests/adapter/test_init.pyThis is a new file. tests/adapter/test_local_adapter.pyThis is a new file. tests/adapter/test_ray_adapter.pyThis is a new file. |
touale
deleted the
70-typeerror-deploymenthandle-object-is-not-callable-after-upgrading-framex-kit-to-012b1
branch
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/adapter/test_local_adapter.py`:
- Around line 189-194: The test triggers a mypy error because
adapter.to_remote_func returns a Callable but the test calls a dynamically
attached attribute wrapped_func.remote which static typing doesn't recognize;
update the test to silence mypy by adding a type ignore on the .remote call
(i.e., add " # type: ignore" to the wrapped_func.remote invocation in the test
that uses adapter.to_remote_func and wrapped_func) so the pattern matches the
existing production code ignores.
🧹 Nitpick comments (3)
tests/adapter/test_local_adapter.py (1)
126-154: Serialization test doesn't actually verify serialization.The test name claims to verify that
_safe_plot_wrapperserializes concurrent calls, but the assertions only check that both calls completed — not that they were serialized (e.g.,call1_endappears beforecall2_startor vice versa). Also,lockon line 130 is unused dead code.♻️ Suggested improvement
def test_safe_plot_wrapper_serializes_calls(self): """Test _safe_plot_wrapper serializes concurrent calls.""" adapter = LocalAdapter() call_order = [] - lock = threading.Lock() # noqa def slow_func(name): call_order.append(f"{name}_start") threading.Event().wait(0.01) # Small delay call_order.append(f"{name}_end") return name # Run two calls concurrently def run_wrapper(name): return adapter._safe_plot_wrapper(slow_func, name) thread1 = threading.Thread(target=run_wrapper, args=("call1",)) thread2 = threading.Thread(target=run_wrapper, args=("call2",)) thread1.start() thread2.start() thread1.join() thread2.join() - # Both calls should complete (in any order due to thread scheduling) - assert "call1_start" in call_order - assert "call1_end" in call_order - assert "call2_start" in call_order - assert "call2_end" in call_order + # Verify serialization: each call's start/end should be adjacent + assert len(call_order) == 4 + # Either call1 ran fully first, or call2 ran fully first + assert call_order in [ + ["call1_start", "call1_end", "call2_start", "call2_end"], + ["call2_start", "call2_end", "call1_start", "call1_end"], + ]tests/adapter/test_ray_adapter.py (1)
23-36: Remove unused blanketnoqadirectives (lines 24-25) as flagged by Ruff.These blanket
noqacomments are no longer suppressing any diagnostics.🔧 Proposed fix
try: - with patch.dict("sys.modules", {"ray": None, "ray.serve": None}): # noqa - with pytest.raises(RuntimeError, match="Ray engine requires extra dependency"): # noqa + with patch.dict("sys.modules", {"ray": None, "ray.serve": None}): + with pytest.raises(RuntimeError, match="Ray engine requires extra dependency"): # Force reload to trigger ImportError import importlibtests/adapter/test_init.py (1)
71-78: Lazy-import test is trivially true and doesn't actually verify the import mechanism.When
use_ray=False,get_adapter()never reaches the code path that touchesRayAdapterat all, somock_ray_module.RayAdapter.assert_not_called()always passes regardless of the patch. This test doesn't meaningfully verify lazy importing — it only confirms theuse_ray=Falsepath doesn't instantiateRayAdapter, which is already covered bytest_get_adapter_returns_local_adapter_when_ray_disabled.Also, the blanket
# noqaon line 73 is unused per Ruff.♻️ Consider replacing with a more meaningful lazy-import test or removing
def test_get_adapter_lazy_imports_ray_adapter(self): """Test that RayAdapter is only imported when needed.""" - with patch("framex.adapter.settings.server.use_ray", False): # noqa - # Import should not happen when use_ray is False - with patch("framex.adapter.ray_adapter") as mock_ray_module: - get_adapter() - # RayAdapter module should not be accessed - mock_ray_module.RayAdapter.assert_not_called() + import sys + # Ensure ray_adapter hasn't been imported as a side effect + with patch("framex.adapter.settings.server.use_ray", False): + get_adapter() + # Verify we got a LocalAdapter without importing RayAdapter class + import framex.adapter as adapter_mod + assert isinstance(adapter_mod._adapter, LocalAdapter)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
CI / Config