-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add Build Matrix for CI #130
Conversation
.github/workflows/build_wheels.yml
Outdated
@@ -1,4 +1,4 @@ | |||
name: run-tests-local | |||
name: build-wheels |
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.
Seems like this was a mistake?
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.
We should still be testing the wheel builds.
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.
Another thought here,
we should add
export SMARTSIM_SUFFIX=develop
prior to the wheel build so the wheels are named with the dirty version.
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.
We are still testing the wheel builds. Wheel builds occur in build_wheels.yml
and the extended build matrix which will eventually contain code coverage reporting is in run_tests.yml
.
I added
env:
SMARTSIM_SUFFIX: "develop"
to the wheel builds in my most recent push.
.github/workflows/run_tests.yml
Outdated
if: "contains( matrix.os, 'macos' )" | ||
run: brew install make || true | ||
|
||
- name: Install SmartSim on MacOS |
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.
we can probably collapse the macos and ubuntu stages here into a single run. This decreases the number of docker layers created by the CI and shoul dmake things faster.
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 changed some stuff here and addressed what I believe you were referring to. Let me know if the change is what you were intending.
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.
LGTM. Glad we discussed this.
* Add Build Matrix for CI (CrayLabs#130) Adds various dimensions to the CI build matrix for SmartSim. The build matrix now uses MacOS & Ubuntu, GNU8, RedisAI 1.2.3 & 1.2.5, and Python 3.7-3.9. The build matrix excludes building with RedisAI 1.2.5 when on MacOS as RedisAI temporarily removed support for MacOS in 1.2.4 and 1.2.5 [ committed by @EricGustin and @Spartee ] [ reviewed by @Spartee ] * Remove numpy as a dependency (CrayLabs#132) * Remove np from step.py and requirements Create a helper function called get_base_36_repr so that we can remove numpy from step.py Remove numpy from requirements.txt Pin requirements.dev to a specific version of numpy * Remove numpy from requirements * Remove numpy from setup [ committed by @EricGustin ] [ reviewed by @Spartee ]
Adds various dimensions to the CI build matrix for SmartSim. The build matrix now uses MacOS & Ubuntu, GNU8, RedisAI 1.2.3 & 1.2.5, and Python 3.7-3.9. The build matrix excludes building with RedisAI 1.2.5 when on MacOS as RedisAI temporarily removed support for MacOS in 1.2.4 and 1.2.5 [ committed by @EricGustin and @Spartee ] [ reviewed by @Spartee ]
This PR adds various dimensions to the CI build matrix for SmartSim. The build matrix now uses MacOS & Ubuntu, GNU8, RedisAI 1.2.3 & 1.2.5, and Python3.7-3.9. The build matrix excludes building with RedisAI 1.2.5 when on MacOS.
Effort and research was conducted on whether the CI can be ran with the Intel compiler. There is a follow up ticket on these findings.
In addition to the CI build matrix, there were modifications to
_core/builder.py
by @Spartee that removed the usage ofshell=True
and replaced it withshell=False
as the default behavior for running commands.