Skip to content
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

Merged
merged 9 commits into from
Aug 1, 2024
Merged

Conversation

congminh1254
Copy link
Member

No description provided.

@coveralls
Copy link

coveralls commented Jul 26, 2024

Pull Request Test Coverage Report for Build 10180423069

Details

  • 81 of 86 (94.19%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 86.569%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/commands/ai/ask.js 27 28 96.43%
src/commands/ai/text-gen.js 36 38 94.74%
src/util.js 18 20 90.0%
Totals Coverage Status
Change from base Build 9403705257: 0.03%
Covered Lines: 4281
Relevant Lines: 4785

💛 - Coveralls

case 'answer':
record.answer = value;
break;
case 'created_at':
Copy link
Contributor

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

}

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?"'];
Copy link
Contributor

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.

multiple: true,
parse(input) {
const record = {};
for (const part of input.split(',')) {
Copy link
Contributor

@arjankowski arjankowski Jul 29, 2024

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?

Comment on lines +1 to +5
{
"answer": "The document is currently in review and awaiting approval.",
"created_at": "2024-07-09T11:29:46.835Z",
"completion_reason": "done"
}
Copy link
Contributor

@mwwoda mwwoda Jul 29, 2024

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

Copy link
Contributor

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?

docs/ai.md Outdated Show resolved Hide resolved
docs/ai.md Outdated Show resolved Hide resolved

AiAskCommand.flags = {
...BoxCommand.flags,
mode: flags.string({
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Comment on lines +1 to +5
{
"answer": "The document is currently in review and awaiting approval.",
"created_at": "2024-07-09T11:29:46.835Z",
"completion_reason": "done"
}
Copy link
Contributor

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/text-gen.js Outdated Show resolved Hide resolved
}

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?"'];
Copy link
Contributor

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

@congminh1254 congminh1254 merged commit 59551d2 into main Aug 1, 2024
12 checks passed
@congminh1254 congminh1254 deleted the support-ai-apis-node branch August 1, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants