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

feat: add e2e test for instructlab CI #174

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

RobotSail
Copy link
Member

This commit adds a modified version of the E2E NVIDIA T4 workflow from the instructlab/instructlab
repository to automatically test new PRs against an NVIDIA box to see how they perform end-to-end.

This PR follows up work from the following PRs:

Signed-off-by: Oleg S [email protected]

@nathan-weinberg nathan-weinberg linked an issue Aug 20, 2024 that may be closed by this pull request
Copy link
Member

@danmcp danmcp left a comment

Choose a reason for hiding this comment

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

LGTM

Note: Looks like the repo has a linting issue that needs to be fixed.

@RobotSail
Copy link
Member Author

@danmcp Thanks, we already fixed it in another PR so will just need to rebase here.

@nathan-weinberg
Copy link
Member

@RobotSail i remember you voiced concerns about the E2E script not living in this repo, is that still the case?

This commit adds a modified version of the E2E NVIDIA T4 workflow from the instructlab/instructlab
repository to automatically test new PRs against an NVIDIA box to see how they perform end-to-end.

Signed-off-by: Oleg S <[email protected]>
@RobotSail
Copy link
Member Author

@nathan-weinberg

i remember you voiced concerns about the E2E script not living in this repo, is that still the case?

Well the current workflow is not ideal but it is less work to have them live there for now until someone is interested in maintaining a separate set of tests in this repo. In the future if we decide to separate repos between upstream & downstream equivalents, the downstream should continue to rely on the tests in the https://github.com/instructlab/instructlab repo. The upstream version, should one exist, would drop this test and would instead rely on its own e2e tests.

@nathan-weinberg
Copy link
Member

@nathan-weinberg

i remember you voiced concerns about the E2E script not living in this repo, is that still the case?

Well the current workflow is not ideal but it is less work to have them live there for now until someone is interested in maintaining a separate set of tests in this repo. In the future if we decide to separate repos between upstream & downstream equivalents, the downstream should continue to rely on the tests in the https://github.com/instructlab/instructlab repo. The upstream version, should one exist, would drop this test and would instead rely on its own e2e tests.

SGTM - we could even have a dedicated repo for upstream tests that other repos could pull from - this was done by the OCP org at one point: https://github.com/openshift/openshift-tests

@nathan-weinberg nathan-weinberg merged commit 9f1f417 into instructlab:main Aug 21, 2024
7 checks passed
@RobotSail
Copy link
Member Author

we could even have a dedicated repo for upstream tests that other repos could pull from - this was done by the OCP org at one point: https://github.com/openshift/openshift-tests

Just to be clear, by upstream here you mean upstream relative to the other OCP repos and not upstream as in the upstream Kubernetes, right?

@nathan-weinberg
Copy link
Member

Yes! So in our case it would be something like:

  • a public repo for InstructLab tests that could be used anywhere upstream for testing within library repos (e.g. instructlab-tests with different tests that each lib can pull from)
  • for tests for downstream projects such as RHEL AI, these tests could be used but the development of them would focus on testing the InstructLab project first and foremost

@RobotSail
Copy link
Member Author

Got it, that makes perfect sense.

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.

Add end-to-end automated test workflow
3 participants