-
Notifications
You must be signed in to change notification settings - Fork 53
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
fix: Update ec2 instance type in unit test workflow file #395
Conversation
719ccf7
to
039c12a
Compare
617a6e4
to
94efa2a
Compare
94efa2a
to
f1fb4bf
Compare
f1fb4bf
to
413e97e
Compare
413e97e
to
d30e45e
Compare
The ec2 instance used to run the unit tests cannot launch because of a configuration issue. Signed-off-by: Courtney Pacheco <[email protected]>
d30e45e
to
c43ede2
Compare
# TEMPORARILY ADDING TESTING BRANCH. WILL REMOVE. | ||
branches: | ||
- "main" | ||
- "release-**" | ||
- "fix-unit-test-workflow-file" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're good with the changes in this file, I will revert these temporary changes
ec2-instance-type: ${{ vars.AWS_REGION }} | ||
# TODO: Update the EC2 instance type to use the EC2 runner variant when GPU calls are mocked | ||
# ec2-instance-type: ${{ env.ec2_runner_variant }} | ||
ec2-instance-type: g4dn.12xlarge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the logic for using this instance type?
subnet-id: subnet-024298cefa3bedd61 | ||
security-group-id: sg-06300447c4a5fbef3 | ||
iam-role-name: instructlab-ci-runner | ||
aws-resource-tags: > | ||
[ | ||
{"Key": "Name", "Value": "instructlab-ci-github-large-runner"}, | ||
{"Key": "Name", "Value": "instructlab-ci-github-unit-test-runner"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name wouldn't match the current pruning. It's currently looking for w+
between github and runner. Either need to adjust here or in the pruning.
@@ -128,10 +134,11 @@ jobs: | |||
aws-region: ${{ vars.AWS_REGION }} | |||
|
|||
- name: "Stop EC2 runner" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't appear to be setup so it will always run the way our other ec2-runner jobs are configured.
Ex: https://github.com/instructlab/instructlab/blob/main/.github/workflows/e2e-nvidia-l4-x1.yml#L150
Where it's a separate step that ways runs and depends on the previous step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering why that 'if(always())' step was in there. I'll add that to my PR #409
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe be something leftover from an old job config - a lot of them have gotten out-of-sync, I had to make two hotfixes to the E2E Custom job in the Core repo today. @courtneypacheco has a Dev Doc open around consolidating some of these into common actions to make CI easier to maintain: instructlab/dev-docs#179
I incorporated these changes into #409 and added @courtneypacheco as the co-author on the commit with those changes. Thanks for your contribution Courtney! |
Resolves #394
Unit tests currently fail due to a minor configuration issue in one of our workflow files. This PR aims to resolve the typo. See referenced git issue.