Adding Todoist toolkit #515
Conversation
| async def get_all_tasks( | ||
| context: ToolContext, | ||
| ) -> Annotated[dict, "The tasks object returned by the Todoist API."]: | ||
| """ | ||
| Get all tasks from the Todoist API. Use this when the user wants to see, list, view, or | ||
| browse all their existing tasks. For getting tasks from a specific project, use | ||
| get_tasks_by_project_name or get_tasks_by_project_id instead. | ||
| """ | ||
|
|
||
| async with httpx.AsyncClient() as client: | ||
| url = get_url(context=context, endpoint="tasks") | ||
| headers = get_headers(context=context) | ||
|
|
||
| response = await client.get(url, headers=headers) | ||
|
|
||
| response.raise_for_status() | ||
|
|
||
| tasks = parse_tasks(response.json()["results"]) | ||
|
|
||
| return {"tasks": tasks} |
There was a problem hiding this comment.
Small nitpick. From Todoist API docs, it seems that the maximum number of tasks returned by default is 50, and they implement a cursor pagination method.
Could we refactor the get_tasks_ tools so they accept the limit and cursor parameters as well? For projects I think this is not necessary, as it seems very unlikely that people will have 50+ projects, but I can see 50+ tasks being reached quite easily.
There was a problem hiding this comment.
A suggestion in this case in terms of code organization, you can have a single utility function that gets tasks, and the more specific tools like get_tasks_by_project_id simply call that function with the correct value populated. In that case, there will be no need to prepare the url and headers in each tool. This is just a suggestion though, and not required for the review!
There was a problem hiding this comment.
Hey @torresmateo - good catch!
From a tool perspective, should I expose limit and cursor as optional or are you suggesting that I always return all potential tasks (even if its +50)?
There was a problem hiding this comment.
I think you should expose the limit and cursor, but perhaps change "cursor" to "next_page_token" (or something similar). If you think 50 is a small number of tasks to return in general, feel free to use a different default value, and add it to the description of the limit parameter. The description of this argument should document:
- the minimum
- the default
- the maximum
and for the cursor (next page token), remember to add it to the response (and the tools descriptions)
There was a problem hiding this comment.
hey @torresmateo ! Just did the change. The utility function I left it on the task file because it's only task-specific but if you think it makes more sense to put it on the utils.py, let me know!
There was a problem hiding this comment.
This is looking very nice! I think the only change I would suggest now (sorry for not addressing this before!) is to combine the "ById" and "ByName" tools into one. LLMs generally function better if you provide fewer tools that do similar things.
As an example:
- GetTasksByProjectId
- GetTasksByProjectName
could be merged into
- GetTasksByProject
and the project parameter can be something like:
project: Annotated[str, "The ID or name of the project to get tasks from."]The implementation then will attempt to look for a project assuming it's an ID, and if that does not exist, search as if it was a name.
What do you think?
There was a problem hiding this comment.
@torresmateo That makes total sense! I've implemented the change and done a couple of restructuring and organized the files and functions. I've added more tests and evals as well.
Let me know how it looks from your side!
|
@torresmateo based on our conversation from Friday, I've updated the tools that produce side effects to require unique params. Tools that don't produce side effects (gets, search) still affects fuzzy params. Let me know if this works! |
3 participants
This PR is adding the Todoist toolkit. The tools that are being added are: