Skip to content

Update document upload api to enhance existing document upload experience #678

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

Merged
merged 1 commit into from
Mar 27, 2025

Conversation

vishnoianil
Copy link
Member

@vishnoianil vishnoianil commented Mar 18, 2025

PR enhances the document upload APIs.

  • All the contributions (knowledge and skill) now given the name
    on creation and not on submit from server side, to drive the
    workflow using the contribution name.
  • User is allowed to see the document only related to that knowledge
    contribution, rather than all the documents across all the
    contributions.
  • Documents are now submitted when user submits the contribution.
  • Existing documents are fetched when user edits the contribution. Both
    newly uploaded documents and the existing documents are presented to
    the user.
  • If user cancel the edit or doesn't submit the contribution, new documents
    are not uploaded and the existing commit sha will remain in the yaml.
  • If user edits the knowledge contribution and adds more documents,
    all the existing and new documents will be shown to the user and
    submitted when user submit the contribution.
  • User don't have to worry about the commit sha anymore. Api
    implementation takes care of collecting all the documents
    and committing them in a single commit, and using that in the yaml.
  • API's also provide option to user to delete any of the existing
    uploaded document, but it's currently is not used. We need to
    design a workflow on how user can manage the contribution related documents.
  • This workflow works similarly for both github and native mode.

Fixes #608

@vishnoianil vishnoianil force-pushed the doc-upload branch 5 times, most recently from c5c65d9 to a5e9bef Compare March 19, 2025 07:55
@vishnoianil vishnoianil marked this pull request as ready for review March 19, 2025 07:57
@vishnoianil
Copy link
Member Author

Screen.Recording.2025-03-19.at.1.08.12.AM.mov

@Misjohns @jeff-phillips-18 This recording contains the demo of the new document upload workflow. PTAL

@vishnoianil vishnoianil force-pushed the doc-upload branch 3 times, most recently from 69af4f2 to 202b637 Compare March 19, 2025 08:33
@Misjohns
Copy link
Collaborator

@vishnoianil The upload docs looks great. Thank you!
I have a few comments on the context selection modal. Should I capture those here or on another issue?

@vishnoianil
Copy link
Member Author

@vishnoianil The upload docs looks great. Thank you!

I have a few comments on the context selection modal. Should I capture those here or on another issue?

Hi @Misjohns , please add it here, if they are minor changes, i can take care of it in this PR, otherwise i will open another issue.

@jeff-phillips-18
Copy link
Collaborator

@vishnoianil Tested and works well.

I'm wondering if, when editing, we should show the already uploaded documents on the Upload documents. This could give the user a) advance information on what docs are available for selecting context from, and b) the ability to remove previously uploaded documents. Thoughts?

@Misjohns
Copy link
Collaborator

@jeff-phillips-18 I agree. If a user chooses to edit a submission, then I'd expect to see the markdown files that were previously created in the submission.

@Misjohns
Copy link
Collaborator

@vishnoianil We already had an existing issue for improving the select context modal. #568

@vishnoianil
Copy link
Member Author

@jeff-phillips-18 @Misjohns Agree on adding the existing files to the list. I was thinking of that as well, and I was also thinking of allowing users to delete the file from the list. For delete It will pop-up a warning with the text similar to "Please ensure that selected contexts does not belong to this document.".

@Misjohns Regarding #568 , let's take it through another PR. This PR is more focussed on the backend APIs, so want to make sure it works with the existing current flow.

@jeff-phillips-18
Copy link
Collaborator

Overall, LGTM. Works well.

…ence

- All the contributions (knowledge and skill) now given the name
  on creation and not on submit from server side, to drive the
  workflow using the contribution name.
- User is only allowed to see the document related to that knowledge
  contribution, rather than all the documents across all the
  contributions.
- Documents are now submitted when user submits the contribution.
- Existing documents are fetched when user edits the contribution. Both
  newly uploaded documents and the existing documents are presented to
  the user.
- If user cancel the edit or doesn't submit the contribution, documents
  are not uploaded and the previous commit sha will remain unchanged in the yaml.
- If user edits the knowledge contribution and adds/removes new/existing documents,
  all the existing and new documents will be shown to the user and
  submitted when user submit the contribution.
- User don't have to worry about the commit sha anymore. Api
  implementation takes care of collecting all the documents
  and committing them in a single commit and using the sha in yaml.
- API's also provide option to user to delete any of the existing
  uploaded document, but it's currently is not used. We need to
  design a workflow on how user can manage the contribution related documents.
- This workflow works similarly for both github and native mode.

Signed-off-by: Anil Vishnoi <[email protected]>
Copy link
Member

@nerdalert nerdalert left a comment

Choose a reason for hiding this comment

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

LGTM!

@nerdalert nerdalert merged commit 3d6040a into instructlab:main Mar 27, 2025
5 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.

Improve the experience of selecting the document for context selection in knowledge form
4 participants