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

Feature gates - #2946

Merged
mergify[bot] merged 1 commit into
instructlab:mainfrom
anastasds:feature-gates
Jan 23, 2025
Merged

Feature gates#2946
mergify[bot] merged 1 commit into
instructlab:mainfrom
anastasds:feature-gates

Conversation

@anastasds

@anastasds anastasds commented Jan 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Related ADR: instructlab/dev-docs#178
Depends on #2886 , #2903
Resolves #2888

  • Adds feature gating following the pattern in the ADR linked above
  • Adds an @dev_preview decorator that mocks the feature scope environment variable for specific tests covering experimental functionality.
  • Adds unit tests verifying that RAG related functionality gives a useful and accurate error message when attempted to be used without being enabled.

@mergify mergify Bot added testing Relates to testing ci-failure PR has at least one CI failure labels Jan 21, 2025
dmartinol
dmartinol reviewed Jan 21, 2025
View reviewed changes

@dmartinol dmartinol 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, apart from some minor comments

Comment thread src/instructlab/feature_flags.py Outdated
Comment thread src/instructlab/cli/rag/convert.py
Comment thread src/instructlab/feature_flags.py Outdated
@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 21, 2025
@mergify

mergify Bot commented Jan 21, 2025

Copy link
Copy Markdown
Contributor

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

@mergify mergify Bot added needs-rebase This Pull Request needs to be rebased ci-failure PR has at least one CI failure labels Jan 21, 2025
@anastasds
anastasds marked this pull request as ready for review January 22, 2025 13:03
@anastasds
anastasds requested a review from a team January 22, 2025 13:03
@mergify mergify Bot removed the needs-rebase This Pull Request needs to be rebased label Jan 22, 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 Jan 22, 2025
bbrowning
bbrowning reviewed Jan 22, 2025
View reviewed changes

@bbrowning bbrowning 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.

Leaving a couple of comments here - nothing that warranted requesting changes on GitHub, but things that could be improved if you're so inclined as well as a typo that we should fix (CustomNoUpgrade value), but that is not in a code path anything would hit yet so is not critical.

Comment thread src/instructlab/cli/rag/convert.py
Comment thread src/instructlab/feature_gates.py Outdated
Comment thread src/instructlab/feature_gates.py
Comment thread src/instructlab/model/chat.py Outdated
@anastasds

anastasds commented Jan 22, 2025

Copy link
Copy Markdown
Contributor Author

I was running just the new feature gating tests and only then realized that some other RAG-related unit tests need to be modified to run properly, so putting this back into draft mode.

@anastasds
anastasds marked this pull request as draft January 22, 2025 14:49
@mergify

mergify Bot commented Jan 22, 2025

Copy link
Copy Markdown
Contributor

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

@mergify mergify Bot added needs-rebase This Pull Request needs to be rebased and removed ci-failure PR has at least one CI failure labels Jan 22, 2025
@anastasds
anastasds marked this pull request as ready for review January 22, 2025 16:03
@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 22, 2025
nathan-weinberg
nathan-weinberg reviewed Jan 22, 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.

Overall looking good to me @anastasds - couple of comments - I'd also like if we can mention this somewhere in the Core CONTRIBUTING.md for future development - I know @jwm4 is planning a revamp of that doc so if he wants to include documenting Feature Gates as part of that work instead of doing so in the PR that's fine with me

Comment thread src/instructlab/cli/rag/convert.py Outdated
Comment thread src/instructlab/model/chat.py Outdated
@cdoern

cdoern commented Jan 22, 2025

Copy link
Copy Markdown
Contributor

@anastasds looks like you'll need to enable the FG to get the unit tests for rag convert to pass

@anastasds

anastasds commented Jan 22, 2025

Copy link
Copy Markdown
Contributor Author

Actually it was because I redid the changes instead of rebasing, and forgot the rag_enabled check for ilab model chat so the logic was broken - fixed and tested locally before pushing.

@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jan 22, 2025
@anastasds
anastasds force-pushed the feature-gates branch 3 times, most recently from 115c286 to a760f50 Compare January 22, 2025 19:27
@mergify mergify Bot added the ci-failure PR has at least one CI failure label Jan 22, 2025
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jan 22, 2025
nathan-weinberg
nathan-weinberg approved these changes Jan 22, 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.

one nit otherwise LGTM

Comment thread tests/test_utils.py
@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jan 22, 2025
@mergify mergify Bot removed the one-approval PR has one approval from a maintainer label Jan 22, 2025
@mergify mergify Bot added the ci-failure PR has at least one CI failure label Jan 22, 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 Jan 22, 2025
@anastasds
anastasds force-pushed the feature-gates branch 2 times, most recently from 5962383 to 3a842de Compare January 22, 2025 21:50
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jan 22, 2025
@mergify mergify Bot added the ci-failure PR has at least one CI failure label Jan 22, 2025
@anastasds
anastasds force-pushed the feature-gates branch 2 times, most recently from 27c7d9b to 2726d3c Compare January 22, 2025 22:25
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jan 22, 2025
@anastasds
Add feature gating
3c6e4c8 
Signed-off-by: Anastas Stoyanovsky <astoyano@redhat.com>
@anastasds anastasds mentioned this pull request Jan 30, 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
@cdoern cdoern cdoern approved these changes
@nathan-weinberg nathan-weinberg nathan-weinberg approved these changes
@dmartinol dmartinol dmartinol 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] Put all RAG related functionality behind feature flag

5 participants

@anastasds @cdoern @bbrowning @nathan-weinberg @dmartinol