-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat: Support AI APIs using Box Node SDK #539
Conversation
Pull Request Test Coverage Report for Build 10180423069Details
💛 - Coveralls |
src/commands/ai/text-gen.js
Outdated
case 'answer': | ||
record.answer = value; | ||
break; | ||
case 'created_at': |
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.
We have a convention in the CLI of using kebab-case (dash-case), so it’s worth adding support for created-at
here, as well as dialogue-history
src/commands/ai/text-gen.js
Outdated
} | ||
|
||
AiTextGenCommand.description = 'Sends an AI request to supported LLMs and returns an answer specifically focused on the creation of new text.'; | ||
AiTextGenCommand.examples = ['box ai:text-gen --dialogue_history=prompt="What is the status of this document?",answer="It is in review" --items=id=12345,type=file --prompt="What is the status of this document?"']; |
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.
The example is missing the created-at
field which is required in the request, so it needs to be added here.
src/commands/ai/text-gen.js
Outdated
multiple: true, | ||
parse(input) { | ||
const record = {}; | ||
for (const part of input.split(',')) { |
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.
In dialogue-history, both the prompt
and answer
can contain the ,
character. In such cases this command will fail because of the split on this character.
Have you considered another solution that would avoid this problem?
{ | ||
"answer": "The document is currently in review and awaiting approval.", | ||
"created_at": "2024-07-09T11:29:46.835Z", | ||
"completion_reason": "done" | ||
} |
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.
The created_at
field have a different structure in generated SDK (it's an object with value field) - https://github.com/box/boxcli/pull/538/files#diff-d8df5e635c0ea2eede968c858e24c4a82154fcb07e0b4907032c3ec70b987435. Do we want to wrap it here or unwrap when using generated SDK to avoid breaking changes? Also the field names are a bit different
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.
@congminh1254 any comment on this one?
src/commands/ai/ask.js
Outdated
|
||
AiAskCommand.flags = { | ||
...BoxCommand.flags, | ||
mode: flags.string({ |
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 think that this param should be removed, as this is always calculated based on the numbers of items.
Can you do this?
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 think we should keep it, it's marked as optional and if user not provided, it's calculated base on numbers of items, but we should provide a solution for user to overwrite it.
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.
Leaving this field as it is will allow the user to provide incorrect input. Specifically, if the user provides one element in the items
parameter but sets the mode
flag to multi_item_qa, or if they provide more than one element in items
but set the mode
to single_item_qa, the request will fail, and the client will receive an error.
In other words, the user should never overwrite this property because it will not work. Therefore, I think leaving it here will cause more harm than good. We also know that this parameter was supposed to be removed from the backend, but due to breaking changes, it has remained and will most likely be removed when we introduce versioning.
{ | ||
"answer": "The document is currently in review and awaiting approval.", | ||
"created_at": "2024-07-09T11:29:46.835Z", | ||
"completion_reason": "done" | ||
} |
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.
@congminh1254 any comment on this one?
src/commands/ai/ask.js
Outdated
} | ||
|
||
AiAskCommand.description = 'Sends an AI request to supported LLMs and returns an answer'; | ||
AiAskCommand.examples = ['box ai:ask --mode single_item_qa --items=id=12345,type=file --prompt "What is the status of this document?"']; |
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.
just remove this --mode signle_item_qa
from the example and regenerate the md file
No description provided.