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 simple and full knowledge pipeline functional tests #466

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bbrowning
Copy link
Contributor

This is a port of the old scripts/test_knowledge.py into functional tests that we can run with CI. These tests need to run on a GPU, so are marked as 'gpu' in pytest and only execute with a new py3-functional-gpu tox environment. This also adds a new workflow file to execute these tests on a GPU runner.

@mergify mergify bot added CI/CD Affects CI/CD configuration testing Relates to testing ci-failure dependencies Pull requests that update a dependency file labels Jan 8, 2025
@mergify mergify bot removed the ci-failure label Jan 8, 2025
@mergify mergify bot added the ci-failure label Jan 8, 2025
@mergify mergify bot added ci-failure and removed ci-failure labels Jan 8, 2025
@mergify mergify bot added ci-failure and removed ci-failure labels Jan 8, 2025
@mergify mergify bot added ci-failure and removed ci-failure labels Jan 8, 2025
@mergify mergify bot added ci-failure and removed ci-failure labels Jan 8, 2025
@mergify mergify bot added ci-failure and removed ci-failure labels Jan 8, 2025
@mergify mergify bot added ci-failure and removed ci-failure labels Jan 8, 2025
@bbrowning bbrowning force-pushed the port_old_tests branch 2 times, most recently from 0c981d2 to c518c82 Compare January 9, 2025 13:43
@mergify mergify bot removed the ci-failure label Jan 9, 2025
@bbrowning
Copy link
Contributor Author

Ok, almost ready to move this out of draft state. For now this only tests our end-to-end SDG simple and full pipelines for knowledge, but will be expanded in the future to cover skills, PDF knowledge docs, etc. The main focus here is to stub in an SDG CI job that runs with GPUs but does not have instructlab/instructlab installed, so that we can start working on a test suite for SDG independent of running InstructLab's e2e tests. We have SDG features to test that won't be exposed via the ilab CLI, and the circular dependency where the core repo depends on SDG but SDG depends on the core repo for its e2e tests makes it harder than it should be to do some types of changes to SDG.

Tagging in @nathan-weinberg and @courtneypacheco as my go-to CI experts. If you have any general feedback on direction or changes to the approach I've taken here to add a new CI job for SDG, I'd love to hear it.

To move this out of draft state I'll want to remove the temporary pull_request workflow trigger added at the top of the workflow file. But, leaving it in there for now in case I need to make any additional changes so that the new CI workflow will run again after I make those changes.

Copy link
Contributor

mergify bot commented Jan 16, 2025

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

@mergify mergify bot added the needs-rebase label Jan 16, 2025
@mergify mergify bot removed the needs-rebase label Jan 16, 2025
@bbrowning bbrowning marked this pull request as draft January 17, 2025 00:29
@bbrowning
Copy link
Contributor Author

Marking this as draft while I use this to track down a fix for #482, since I was originally able to reproduce that problem here.

This is a port of the old `scripts/test_knowledge.py` into functional
tests that we can run with CI. These tests need to run on a GPU, so are
marked as 'gpu' in pytest and only execute with a new py3-functional-gpu
tox environment. This also adds a new workflow file to execute these
tests on a GPU runner.

Signed-off-by: Ben Browning <[email protected]>
@bbrowning bbrowning marked this pull request as ready for review January 20, 2025 18:46
@bbrowning
Copy link
Contributor Author

Ok, all tests pass and removed the temporary dispatch conditions that allowed me to test and trigger this before merging. It's ready for review and should be setup for proper dispatch workflow post-merging now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Affects CI/CD configuration dependencies Pull requests that update a dependency file testing Relates to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant