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

gh/actions unit test workflows #384

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

JamesKunstle
Copy link
Contributor

@JamesKunstle JamesKunstle commented Jan 6, 2025

Adds a workflow to run our unit tests via tox on an EC2 runner. Runs:

  1. When a PR is opened, synchronized, and re-opened.
  2. When there is a push to the main or 'release-' branches.

@mergify mergify bot added CI/CD Affects CI/CD configuration testing Relates to testing ci-failure labels Jan 6, 2025
@JamesKunstle JamesKunstle force-pushed the add-unit-workflow branch 6 times, most recently from 37621af to c18287f Compare January 6, 2025 23:23
@mergify mergify bot removed the ci-failure label Jan 6, 2025
@nathan-weinberg
Copy link
Member

nathan-weinberg commented Jan 7, 2025

@courtneypacheco @bjhargrave could y'all PTAL at this as well?

.github/workflows/unit-nvidia-reusable.yaml Outdated Show resolved Hide resolved
.github/workflows/unit-nvidia-reusable.yaml Outdated Show resolved Hide resolved
.github/workflows/unit-nvidia-reusable.yaml Outdated Show resolved Hide resolved
src/instructlab/training/config.py Show resolved Hide resolved
Copy link

@courtneypacheco courtneypacheco left a comment

Choose a reason for hiding this comment

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

Had a few questions besides Nathan's, but looks good otherwise!

tests/test_init.py Show resolved Hide resolved
.github/workflows/unit-nvidia-reusable.yaml Outdated Show resolved Hide resolved
.github/workflows/unit-nvidia-reusable.yaml Outdated Show resolved Hide resolved
.github/workflows/unit-nvidia-reusable.yaml Outdated Show resolved Hide resolved
.github/workflows/unit-nvidia-reusable.yaml Outdated Show resolved Hide resolved
.github/workflows/unit-nvidia-fast-l40s-x4.yaml Outdated Show resolved Hide resolved
.github/workflows/unit-nvidia-reusable.yaml Outdated Show resolved Hide resolved
@JamesKunstle JamesKunstle force-pushed the add-unit-workflow branch 5 times, most recently from c9ebf6e to b72ac75 Compare January 9, 2025 00:02
@JamesKunstle
Copy link
Contributor Author

@danmcp @nathan-weinberg @bjhargrave Could you please look at the failure that the job is getting and help figure out why the workflow can't access vars.AWS_REGION? I've played around with it for a while and I can't figure out why the vars context doesn't have anything in it. I'm adding a debugging step to show that the variable is indeed empty.

@danmcp
Copy link
Member

danmcp commented Jan 9, 2025

@danmcp @nathan-weinberg @bjhargrave Could you please look at the failure that the job is getting and help figure out why the workflow can't access vars.AWS_REGION? I've played around with it for a while and I can't figure out why the vars context doesn't have anything in it. I'm adding a debugging step to show that the variable is indeed empty.

It's not going to be able to access the variable from a fork PR with a new workflow.

@JamesKunstle
Copy link
Contributor Author

@danmcp Oh, duh. Of course it can't.

@JamesKunstle JamesKunstle requested a review from danmcp January 9, 2025 19:43
@JamesKunstle JamesKunstle self-assigned this Jan 9, 2025
@alimaredia alimaredia requested review from alimaredia and removed request for bjhargrave, courtneypacheco and alimaredia January 9, 2025 21:27
@JamesKunstle JamesKunstle requested review from bjhargrave and courtneypacheco and removed request for bjhargrave January 9, 2025 22:21
@mergify mergify bot removed the one-approval label Jan 9, 2025
@mergify mergify bot merged commit 862e992 into instructlab:main Jan 9, 2025
16 of 17 checks passed
@JamesKunstle JamesKunstle deleted the add-unit-workflow branch January 10, 2025 06:45
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 ci-failure testing Relates to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants