-
Notifications
You must be signed in to change notification settings - Fork 78
feat: cancellable Actor run #228
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
Conversation
|
We could add a test to check that cancelled requests don't send any response. Now is only tested than request is cancelled. |
MQ37
left a comment
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.
As I already menitoned in person, please change the Actor in tests to either apify/python-example or apify/rag-web-browser. These are already on staging and I would prefer to not add a new Actor there to our testing suite. If that is not possible we can include this Actor (if truly needed). Otherwise LGTM, thank you 👍
jirispilka
left a comment
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.
I agree with @MQ37. I also left a couple of comments that should be taken care of.
It should also be possible to send the cancel/notification to actor-mcp, but let’s not complicate this issue with that.
Please create a new issue so we don’t forget.
|
I also need to check if the Actorized MCP server receives a cancel notification. I could try to make some tests, but it can be hard/impossible with actual source code. |
jirispilka
left a comment
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.
I left a couple of questions, if you can please address them
|
I tested the cancellation of the job working for the actor and the call-actor tool. Checked with VSCode. I found a bug where the client in test is not closed when some test fails. It's fixed when the client is always closed in |
jirispilka
left a comment
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.
I haven't checked the tests ... the rest look good to me. Thanks
Closes: #160
Tested in:
I tried to write a test using manually sending a cancel notification, but I need a request ID, and I don't know where to find it because it's managed internally in the MCP client.