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: MVP Knowledge #1256

Closed
wants to merge 218 commits into from
Closed

feat: MVP Knowledge #1256

wants to merge 218 commits into from

Conversation

thewbuk
Copy link
Contributor

@thewbuk thewbuk commented Jun 20, 2024

Because

  • The first 3 tabs in the Knowledge section

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (2 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

9 similar comments
Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (2 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (2 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (2 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (2 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (2 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (2 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (2 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (2 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (2 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (1 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (2 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link

📦 Next.js Bundle Analysis for instillai-console

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 77.64 KB (3 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link
Member

@EiffelFly EiffelFly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thewbuk The code you written is keeping up the code quality we want, that is pretty good! But I want to address several points with you

  • Don't destruct the react-query return since it hurt readability
  • Please put the query in package/sdk and maintain it there. You can reference how I wrote the client there, should be easy to replicate
  • Use Nullable everywhere if you need null data type
  • Reduce drastically the frequency of using setTimeout in the react lifeCycle. That is not a good pattern to escape from react lifeCycle and will cause many maintainance difficulties afterward.

Please fix these issues, and I will come back to review them further 👍

Comment on lines 21 to 24
accessToken={accessToken.isSuccess ? accessToken.data : null}
enableQuery={accessToken.isSuccess}
router={router}
/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thewbuk We don't use this kind of props to input access token and enabledQuery now. We can use the state in the useInstillStore. Please take a look at how we make it in other pages

packages/design-system/src/new-ui/Icons/SlashDivider.tsx Outdated Show resolved Hide resolved
Comment on lines 18 to 26
if (!accessToken) {
return Promise.reject(new Error("accessToken not provided"));
}
const client = createInstillAxiosClient(accessToken, true);
const response = await client.post<{
knowledge_base: KnowledgeBase;
}>(`/namespaces/${ownerId}/knowledge-bases`, payload);
return response.data.knowledge_base;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thewbuk We usually maintain the query separately in the lib/vdp-sdk and now in packages/sdk

Please mimic the style in the sdk packages

You can have a arifact folder under the packages/sdk/src and put all the query in the ArtifactClient.

In this way, our user can also have a sdk about artifact to use, if they need it

PS: All the react-query-services have this issue

Comment on lines 30 to 34
let APIVersion = process.env.NEXT_PUBLIC_GENERAL_API_VERSION;

if (isModelEndpoint) {
APIVersion = env("NEXT_PUBLIC_MODEL_API_VERSION");
if (isAlphaVersion) {
APIVersion = process.env.NEXT_PUBLIC_MODEL_API_VERSION;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reference how we set up a separate APIClient using the new sdk here

export function getInstillModelAPIClient({

I believe we also need to have a getInstillArtifactAPIClient here

packages/toolkit/src/lib/vdp-sdk/knowledge/types.ts Outdated Show resolved Hide resolved
</Form.Item>
)}
/> */}
<div className="mt-8 flex justify-end gap-x-3">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thewbuk I will encourage you to use one direction margin only, and in most case in this repo, use margin-bottom

Comment on lines +60 to +63
setTimeout(() => {
onClose();
reset(initialValues);
}, 1000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thewbuk What is the reason to use setTimeout here?

Comment on lines +34 to +48
const { data: fileContent, isLoading: isLoadingContent } = useGetFileContent({
fileUid,
kbId,
accessToken,
enabled: isOpen,
ownerId,
});

const { data: chunks, isLoading: isLoadingChunks } = useListChunks({
kbId,
accessToken,
enabled: isOpen && highlightChunk,
ownerId,
fileUid,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thewbuk Please don't destruct the react-query return, or you need to keep casting new name for each state

@thewbuk thewbuk force-pushed the wojciech/knowledge-base-upload-file branch from 7be54c3 to 1f37469 Compare July 24, 2024 20:15
Copy link
Member

@EiffelFly EiffelFly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thewbuk There have multiple issues for your sdk and react-query

  • For the sdk, the request and response type need to be exported so you need to give them a type name
  • For react-query services, the query function is not right, please double check how we write the pipeline and model query fn

Comment on lines 110 to 146
const cloneKnowledgeBaseMutation = useMutation({
mutationFn: async (knowledgeBase: KnowledgeBase) => {
if (!accessToken || !me.data?.id) {
throw new Error("Not authenticated");
}

const client = getInstillAPIClient({ accessToken });
return client.vdp.knowledgeBase.createKnowledgeBase({
ownerId: me.data.id,
payload: {
name: `${ knowledgeBase.name } -clone`,
description: knowledgeBase.description,
tags: knowledgeBase.tags || [],
},
});
},
onSuccess: () => {
queryClient.invalidateQueries(['knowledgeBases', me.data?.id]);
},
});

const deleteKnowledgeBaseMutation = useMutation({
mutationFn: async (kbId: string) => {
if (!accessToken || !me.data?.id) {
throw new Error("Not authenticated");
}

const client = getInstillAPIClient({ accessToken });
return client.vdp.knowledgeBase.deleteKnowledgeBase({
ownerId: me.data.id,
kbId,
});
},
onSuccess: () => {
queryClient.invalidateQueries(['knowledgeBases', me.data?.id]);
},
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thewbuk We need to move these mutations into react-query-services

packages/sdk/src/vdp/artifact/ArtifactClient.ts Outdated Show resolved Hide resolved
packages/sdk/src/vdp/artifact/ArtifactClient.ts Outdated Show resolved Hide resolved
packages/sdk/src/vdp/artifact/ArtifactClient.ts Outdated Show resolved Hide resolved
packages/sdk/src/vdp/artifact/ArtifactClient.ts Outdated Show resolved Hide resolved
@thewbuk thewbuk closed this Aug 1, 2024
@EiffelFly EiffelFly deleted the wojciech/knowledge-base-upload-file branch August 13, 2024 11:04
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.

2 participants