Skip to content
This repository was archived by the owner on Apr 23, 2026. It is now read-only.

feat(rag): Add lab rag convert CLI command - #2902

Merged
mergify[bot] merged 1 commit into
instructlab:mainfrom
jwm4:jwm4-rag-convert
Jan 17, 2025
Merged

feat(rag): Add lab rag convert CLI command#2902
mergify[bot] merged 1 commit into
instructlab:mainfrom
jwm4:jwm4-rag-convert

Conversation

@jwm4

@jwm4 jwm4 commented Jan 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Adds new ilab rag convert command which takes in input documents from a taxonomy or a directory of original document files (e.g., PDFs) and outputs Docling JSON for each input document.

Issue resolved by this Pull Request:
Resolves #2890

Dev Docs related to this Pull Request:
Link to Dev Doc or PR: instructlab/dev-docs#161

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Changelog updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Functional tests have been added, if necessary.
  • E2E Workflow tests have been added, if necessary.

@mergify

mergify Bot commented Jan 10, 2025

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. @jwm4 please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added testing Relates to testing needs-rebase This Pull Request needs to be rebased ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jan 10, 2025
bbrowning
bbrowning reviewed Jan 13, 2025
View reviewed changes
Comment thread src/instructlab/rag/process_docs.py Outdated
dmartinol
dmartinol reviewed Jan 14, 2025
View reviewed changes
Comment thread src/instructlab/rag/process_docs.py Outdated
dmartinol
dmartinol reviewed Jan 14, 2025
View reviewed changes
Comment thread src/instructlab/cli/rag/convert.py
dmartinol
dmartinol reviewed Jan 14, 2025
View reviewed changes
Comment thread src/instructlab/cli/rag/convert.py Outdated
dmartinol
dmartinol reviewed Jan 14, 2025
View reviewed changes
Comment thread src/instructlab/cli/rag/rag.py Outdated
dmartinol
dmartinol reviewed Jan 14, 2025
View reviewed changes
Comment thread src/instructlab/rag/process_docs.py Outdated
dmartinol
dmartinol reviewed Jan 14, 2025
View reviewed changes
Comment thread src/instructlab/rag/process_docs.py Outdated
dmartinol
dmartinol reviewed Jan 14, 2025
View reviewed changes
Comment thread src/instructlab/rag/process_docs.py Outdated
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed needs-rebase This Pull Request needs to be rebased ci-failure PR has at least one CI failure labels Jan 15, 2025
cdoern
cdoern reviewed Jan 16, 2025
View reviewed changes

@cdoern cdoern left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just reviewing the tests. These align with the team's expectations of basic unit tests for a new implementation, no major concerns from me

Comment thread tests/test_lab_rag_convert.py
Comment thread tests/test_lab_rag_convert.py Outdated
Comment thread tests/test_lab_rag_convert.py
dmartinol
dmartinol reviewed Jan 16, 2025
View reviewed changes
Comment thread tests/test_lab_rag_convert.py Outdated
dmartinol
dmartinol reviewed Jan 16, 2025
View reviewed changes
Comment thread tests/test_lab_rag_convert.py
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jan 16, 2025
@nathan-weinberg
nathan-weinberg self-requested a review January 16, 2025 15:21
@jwm4
jwm4 marked this pull request as ready for review January 16, 2025 19:31
@jwm4
jwm4 requested a review from a team January 16, 2025 19:42
@jwm4 jwm4 changed the title [Draft] ilab rag convert feat(rag): Add lab rag convert CLI command Jan 16, 2025
@jwm4
jwm4 force-pushed the jwm4-rag-convert branch from 180a27e to 1f01cf2 Compare January 16, 2025 21:04
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jan 16, 2025
@jwm4
jwm4 marked this pull request as draft January 16, 2025 21:06
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jan 16, 2025
@jwm4
jwm4 marked this pull request as ready for review January 17, 2025 00:58
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jan 17, 2025
nathan-weinberg
nathan-weinberg approved these changes Jan 17, 2025
View reviewed changes

@nathan-weinberg nathan-weinberg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have one question for Charlie about the testdata but otherwise this is looking good to me - approving from side

Comment thread tests/testdata/default_config.yaml
@nathan-weinberg nathan-weinberg added the hold In-progress PR. Tag should be removed before merge. label Jan 17, 2025
@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jan 17, 2025
cdoern
cdoern reviewed Jan 17, 2025
View reviewed changes

@cdoern cdoern left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nothing major, I appreciate using the new config_class and the added comments to some of the existing assertions, doc strings, etc.

Comment thread src/instructlab/cli/rag/convert.py Outdated
Comment thread src/instructlab/cli/rag/convert.py Outdated
Comment thread src/instructlab/cli/rag/convert.py Outdated
Comment thread src/instructlab/clickext.py
Comment thread src/instructlab/rag/convert.py
Comment thread src/instructlab/rag/convert.py
cdoern
cdoern approved these changes Jan 17, 2025
View reviewed changes

@cdoern cdoern left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm. @jwm4 can you squash the commits and then I will remove the hold?

@mergify mergify Bot removed the one-approval PR has one approval from a maintainer label Jan 17, 2025
@jwm4
feat(rag): Add lab rag convert CLI command
be9101b 
Signed-off-by: Bill Murdock <bmurdock@redhat.com>
@jwm4
jwm4 force-pushed the jwm4-rag-convert branch from 306054b to be9101b Compare January 17, 2025 18:07
@jwm4

jwm4 commented Jan 17, 2025

Copy link
Copy Markdown
Contributor Author

Commits are squashed now.

@jwm4 jwm4 removed the hold In-progress PR. Tag should be removed before merge. label Jan 17, 2025
@mergify
mergify Bot merged commit bc784d7 into instructlab:main Jan 17, 2025
@jwm4 jwm4 mentioned this pull request Jan 17, 2025
Closed
@dmartinol dmartinol mentioned this pull request Jan 20, 2025
Closed
11 tasks
@jwm4
jwm4 deleted the jwm4-rag-convert branch February 12, 2025 13:52
@booxter booxter mentioned this pull request Apr 2, 2025
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers
@bbrowning bbrowning bbrowning left review comments
@nathan-weinberg nathan-weinberg nathan-weinberg approved these changes
@dmartinol dmartinol dmartinol left review comments
@cdoern cdoern cdoern approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

testing Relates to testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[RAG][Dev] Implement ilab rag convert

5 participants

@jwm4 @bbrowning @cdoern @nathan-weinberg @dmartinol