-
Notifications
You must be signed in to change notification settings - Fork 33
feat: Insertion functionality #45
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
base: main
Are you sure you want to change the base?
Conversation
✅ DCO Check Passed Thanks @Ryfernandes, all your commits are properly signed off. 🎉 |
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/
|
@ceberam This latest commit is the way I structured the insertion tools for the local fork that I used in the Docling-Client demo. It's structured in a way to minimize the number of tools by combining the insertion and addition functionalities into one tool. This does make the parameters more complex, but Claude was able to handle it fine after some tweaking of the docstring/parameter annotations. I haven't tested with smaller models, so do you think they would be able to handle this type of tool calling reliably? |
Signed-off-by: Ryan Fernandes <[email protected]>
I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: 24b5ae7 I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: 3297d17 Signed-off-by: Ryan Fernandes <[email protected]>
I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: 24b5ae7 I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: 3297d17 I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: 10c7588 Signed-off-by: Ryan Fernandes <[email protected]>
d5a74d7
to
439ac5f
Compare
I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: 14af47a I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: 17de1fc I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: 58760e1 Signed-off-by: Ryan Fernandes <[email protected]>
I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: 14af47a I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: 17de1fc I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: 58760e1 I, Ryan Fernandes <[email protected]>, hereby add my Signed-off-by to this commit: ee1b802 Signed-off-by: Ryan Fernandes <[email protected]>
Signed-off-by: Ryan Fernandes <[email protected]>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
This is great! 🏆
Issue resolved by this Pull Request:
Resolves #43
The primary feature of this pull request is to refactor most of the tools in generation.py. Before, these tools allowed node items to be appended to the end of the document. Now, they have been refactored to enable both append and insert operations. This is achieved through two parameters: sibling_anchor and parent_anchor. The control flow works as follows:
This is more complex than the old tools, but it allows for much more functionality without extra tool definitions. Based on tests with Claude as a client, frontier LLMs are able to comprehend the following directions in the tool docstring:
In addition to the insertion functionality, I have also refactored the way that lists work in docling-mcp. This refactor was proposed in the original issue and is what I implemented for my Docling MCP client, but I am happy to discuss further. Essentially, the old list system used local_stack_cache to track open/closed lists. Instead of having this auxiliary system, which only functions for one list at a time at the end of the document, I changed the code to be more consistent with the ways that lists work in docling-core: enabling list items to be added as children of specified list group parents. With this, I turned the open/close list group tools into a single
add_list_group_to_docling_document
tool. Additionally, all of the stack checks and furniture items have been removed. The safeguards to prevent things like titles in list groups and list items outside of list groups still exist.Lastly, for all tools that insert/append node items, I added an additional value to the returned object: the document anchor(s) of the newly created object(s). From limited testing (moreso speculation) this should help the client to place multiple new items consecutively, without having to make intermediate
get_overview_of_document_anchors
calls.These changes have been tested with Claude as a client, but still should be tested on LlamaStack. Once we confirm that this new format is not too complex, I can quickly add some regression tests before merge.
Claude Tests (New features and "regression" from #40)
Test 1: Confirm that previous conversation flows still work and insert a new paragraph (test robustness by not giving the exact position/document anchor, instead referring to a relational position)
Markdown file:
example_editing_converted.md
Test 2: Confirm that documents can still be created sequentially, test the insertion of multiple items at once (prompts purposefully conversational/ambiguous at times)
Markdown file:
example_justin_bieber.md
Test 3: Confirm that lists can still be created and have items inserted after creation
Markdown file:
example_list_operations.md