Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Comments

Close side panel

fix: Added default embedding model to models downloaded by default#3118

Merged
mergify[bot] merged 1 commit intoinstructlab:maininstructlab/instructlab:mainfrom
dmartinol:fix_2961dmartinol/instructlab:fix_2961Copy head branch name to clipboard
Feb 4, 2025
Merged

fix: Added default embedding model to models downloaded by default#3118
mergify[bot] merged 1 commit intoinstructlab:maininstructlab/instructlab:mainfrom
dmartinol:fix_2961dmartinol/instructlab:fix_2961Copy head branch name to clipboard

Conversation

@dmartinol
Copy link
Contributor

  • Added default embedding model to list of models downloaded by default
  • Adapter existing UT to new behavior
  • According to the decisions tracked in the related issue ... restricting this behavior to apply only when the feature flag for RAG is turned on ..., the embedding model is always downloaded, no matter the configuration of the feature flags.

Issue resolved by this Pull Request:
Resolves #2961

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 added testing Relates to testing ci-failure PR has at least one CI failure labels Feb 3, 2025
@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 Feb 3, 2025
@mergify mergify bot removed the ci-failure PR has at least one CI failure label Feb 3, 2025
@dmartinol dmartinol marked this pull request as ready for review February 3, 2025 12:51
@jpodivin
Copy link
Contributor

jpodivin commented Feb 3, 2025

You shouldn't need changes to the qna.yaml file. Those were part of #3115

Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
@dmartinol
Copy link
Contributor Author

You shouldn't need changes to the qna.yaml file. Those were part of #3115

Thanks! I just rebased and it was removed from the PR.

@jaideepr97
Copy link
Contributor

why not just add the --rag flag to ilab model download to have it download the embedding model for the rag use case?

@dmartinol
Copy link
Contributor Author

  • restricting this behavior to apply only when the feature flag for RAG is turned on

I thought it was covered by the discussion in the related issue about restricting this behavior to apply only when the feature flag for RAG is turned on, but maybe better to ask @jwm4 for confirmation.

@jwm4
Copy link
Contributor

jwm4 commented Feb 3, 2025

I still prefer the approach I specified in the issue:

There was some discussion of restricting this behavior to apply only when the feature flag for RAG is turned on. However, this is problematic if users first run ilab model download with default settings and then turn on the feature flag for RAG and then try to run RAG with default settings. Also, having conditional default values for parameters is just inherently complicated and confusing. Furthermore, the model doesn't take up a huge amount of disk space (under 0.5 GB) so it doesn't seem like a big problem is some users wind up downloading an extra model that they don't wind up using.

However, I understand it is debatable, and I am happy to reconsider if there is a broad consensus that this is not the right approach.

@jwm4 jwm4 requested a review from a team February 3, 2025 19:29
Copy link
Contributor

@jwm4 jwm4 left a comment

Choose a reason for hiding this comment

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

I understand there is still some disagreement about whether this is the right UX, but it is still the one I prefer so I am approving the PR.

Copy link
Contributor

@jaideepr97 jaideepr97 left a comment

Choose a reason for hiding this comment

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

approving based on previously conducted discussions

@mergify mergify bot added the one-approval PR has one approval from a maintainer label Feb 4, 2025
Copy link
Contributor

@booxter booxter left a comment

Choose a reason for hiding this comment

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

Prefer this to adding code to handle --rag option. Especially with the other stuff going on that may affect the future of CLI.

@mergify mergify bot merged commit a289153 into instructlab:main Feb 4, 2025
27 checks passed
@mergify mergify bot removed the one-approval PR has one approval from a maintainer label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Relates to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RAG][Dev] Default embedding model for RAG downloads by default in ilab model download

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.