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

Add knowledge form context selection #439

Merged
merged 3 commits into from
Jan 11, 2025

Conversation

nerdalert
Copy link
Member

@nerdalert nerdalert commented Dec 18, 2024

  • Adds routes for retrieving files from the local knowledge git
  • Adds the ability to view the file changes/additions in the native dashboard.
  • Enables the user to view the knowledge document, highlight the text they want to add to the context field and submit it. The highlighted text will get populated to the context field.
  • Allows the user to still manually enter context or knowledge file details and handles the state switching when they choose to do so.
  • Prevents the user from selecting context from a different commit SHA if they have already selected context from another SHA.
  • Lets the user toggle all files in the knowledge files repo if they wanted to use a different file/SHA for context if they want to. Otherwise, the default is the latest commit. Probably the ideal scenario would be to return the SHA from the /api/native/git/upload or whatever the route is to return the SHA that could then be referenced and present the user with an empty state if it isn't defined but that would be a lot more code to an already too busy PR.

Note: this probably breaks the container deploy for native with the changes in src/app/api/native/upload/route.ts. It's not clear to me how to rebase into that if someone could take a look. I think @vishnoianil did that if you don't mind good sir.

Not an easy review, there are a ton of different states getting munged together here UploadYaml/AutoFillYaml/DownloadYaml/ViewYaml/ManualKnowledgeFile to name a few. Did my best to juggle all of them but will def have some bugs to fix.

Demo vid:

knowledge-context-v1.mp4

@nerdalert nerdalert force-pushed the ctx-select branch 2 times, most recently from 73a6561 to 5c54128 Compare December 18, 2024 07:00
@vishnoianil
Copy link
Member

@nerdalert I will get to this sometime tomorrow.

@nerdalert
Copy link
Member Author

Resolved the rebase issue I had by changing ENV values. Should be good to go meow.

Copy link
Member

Choose a reason for hiding this comment

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

@nerdalert I don't see devLog used anywhere in this PR, probably remove it?

{alertInfo.message}
</Alert>
</AlertGroup>
<Alert variant={alertInfo.type} title={alertInfo.title} actionClose={<AlertActionCloseButton onClose={() => setAlertInfo(undefined)} />}>
Copy link
Member

Choose a reason for hiding this comment

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

We can switch the toast alert, so that all the alerts are shown in the top right corner. Here is the example https://github.com/instructlab/ui/blob/main/src/components/Contribute/Skill/Native/index.tsx.

Not required for this PR, but something we can start using for all future alert.

Copy link
Member

@vishnoianil vishnoianil left a comment

Choose a reason for hiding this comment

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

LGTM

- Adds routes for retrieving files from the local knowledge git
- Adds the ability to view the file changes/additions in the
native dashboard.
- Enables the user to view the knowledge document, highlight the
text they want to add to the context field and submit it. The highlighted
text will get populated to the context field.
- Allows the user to still manually enter context or knowledge file details
and handles the state switching when they choose to do so.
- Prevents the user from selecting context from a different commit SHA if
they have already selected context from another SHA.

Signed-off-by: Brent Salisbury <[email protected]>
There is some chatty logging that will be helpful in the short term
but should be limited to node dev mode.

Signed-off-by: Brent Salisbury <[email protected]>
- Also change the field name to include server side filepath.

Signed-off-by: Brent Salisbury <[email protected]>
@nerdalert nerdalert merged commit ab194ec into instructlab:main Jan 11, 2025
6 checks passed
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