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

Fix issues encountered during pytest #166

Merged
merged 44 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
1afacc2
Merge branch 'fastsim-3' into f3/fix-pytest-issues
Nov 15, 2024
11b0162
Add source dist and wheel builds; save artifacts.
Nov 15, 2024
229d4f0
Fix typo with quotes
Nov 15, 2024
4f6f2b8
Fix issue with string replacement.
Nov 15, 2024
9ca2e11
Go back to using just python minor version.
Nov 15, 2024
abd68a4
Update build_and_test file to include pytest for test_speedup.py
Nov 18, 2024
c5660d0
Disable speed tests without environment variable
Nov 18, 2024
4a913ce
Update for both x86 as well as apple silicon targets
Nov 18, 2024
501b2c6
Turn speedup tests off by default
Nov 18, 2024
3cf1bd3
Conditionally add target architecture
Nov 18, 2024
cee6005
Add required path to tests for cibuildwheel to work
Nov 18, 2024
1bdd93d
Revert to unittest suite (vs pytest)
Nov 18, 2024
9608026
Iterating on build settings for cibuildwheel
Nov 18, 2024
2f77822
Combine target names and set artifact names.
Nov 18, 2024
1261f67
Temporarily reduce matrix size
Nov 18, 2024
0573991
Reduce run matrix
Nov 18, 2024
c86f6db
Improve artifact names; try macos CI
Nov 18, 2024
3d95985
Correct syntax error in YAML
Nov 18, 2024
e7a99b9
Adjust settings to get CI to build MacOS X
Nov 18, 2024
0823398
Add back MacOS arm64
Nov 18, 2024
2184505
Switch to arm64
Nov 18, 2024
153cc5b
Set up Python to use native python for macos (Apple Silicon)
Nov 18, 2024
435f605
Try reverting cibuildwheel command to FS2
Nov 18, 2024
afc953a
Update workflow to try to get macos to build
Nov 19, 2024
b8a423c
Differentiate artifact names
Nov 19, 2024
c55a880
Run with full matrix
Nov 20, 2024
e3b200d
Revert to closer to fastsim-2 workflow
Nov 20, 2024
51bd2ef
Switch to new Github Action for rust installation
Nov 20, 2024
f56d144
Correct matrix variable path
Nov 21, 2024
1d10eab
Add os version to build string
Nov 21, 2024
5051c11
Adjust macos build configs for rust
Nov 21, 2024
5b274c2
Run only macos-14 for testing; change version from number to string
Nov 22, 2024
706178f
Add x86_64 for MacOS-14
Nov 22, 2024
8dace69
Explicitly set MacOS arch vs universal2.
Nov 22, 2024
6dc3c23
Include macos-13
Nov 22, 2024
8802214
Re-enable all os/python versions
Nov 22, 2024
d9238df
Update Python versions to test
Nov 22, 2024
a25a117
Update python versions to include 3.11
Nov 22, 2024
a83a9eb
Bump min version of python to range tested.
Dec 4, 2024
79d4ded
Set minimum MacOS deployment target for Intel mac builds
michael-okeefe Dec 4, 2024
cfd75ed
Merge branch 'fastsim-3' into f3/fix-pytest-issues
calbaker Dec 5, 2024
ef074a6
Merge branch 'fastsim-3' of github.com:NREL/fastsim into f3/fix-pytes…
calbaker Dec 5, 2024
83e16d4
Add note for os matrix
michael-okeefe Dec 9, 2024
9249823
Switch to use latest vs 14 for macos
michael-okeefe Dec 9, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 93 additions & 16 deletions .github/workflows/build_and_test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,111 @@ on: [push]

jobs:
build-and-test:
name: build ${{ matrix.python-version }} on ${{ matrix.platform || matrix.os }}
name: build py3.${{ matrix.python-version }} on ${{ matrix.platform || matrix.os.base }}-${{ matrix.os.version }}
strategy:
fail-fast: true
# NOTE: In the matrix below, we call out macos 13 and latest (> 13) explicitly. This is to enable running on both Intel and Apple Silicon for Mac.
# As of this writing, MacOS 13 is Intel and MacOS 14 and above is Apple Silicon. Ideally, the Apple transition to their new chip occurs fast and
# we can drop macos-13 checks and then just use 'latest'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@michael-okeefe , could we use 13 and latest so that we don't have to remember to increment the 14?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@calbaker Great idea. I made the change and the tests are running.

matrix:
os:
- ubuntu
- macos
- windows
- base: ubuntu
version: latest
- base: macos
version: '13'
Copy link
Collaborator

Choose a reason for hiding this comment

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

@michael-okeefe , could you drop some comments in here explaining why latest does not work for macos and why 13 and 14 are used here? I want to make sure this is not hard to decipher in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@michael-okeefe , please also explain in the comments what event should trigger adding 15, 16, ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I can do that. The issue is related to what processor each github CI machine is running for mac. MacOS 13 is Intel and MacOS 14 is Apple Silicon. Ideally, the Apple transition to their new chip occurs fast and we can drop having to call out specific machines and then just use latest, etc. 15, 16, etc. can be brought in at any time we desire and they should look like the calls for 14. As soon as 13 drops from the board, we can go back to just saying 'latest'.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@calbaker Note added to clarify why we are calling out macos-13 and 14.

arch: 'x86_64'
- base: macos
version: 'latest'
arch: 'arm64'
- base: windows
version: latest
arch: 'amd64'
python-version:
- "3.8"
- "3.9"
- "3.10"
runs-on: ${{ format('{0}-latest', matrix.os) }}
- "10"
- "11"
include:
- os:
base: ubuntu
version: latest
platform: linux
- os:
base: windows
version: latest
ls: dir
runs-on: ${{ format('{0}-{1}', matrix.os.base, matrix.os.version) }}
steps:
- uses: actions/checkout@v4

- name: set up rust
uses: dtolnay/rust-toolchain@stable
- run: rustup target add aarch64-apple-darwin
if: matrix.os == 'macos'

- name: install rust target for MacOS 14 (Apple Silicon)
run: rustup target add aarch64-apple-darwin x86_64-apple-darwin
if: matrix.os.base == 'macos' && matrix.os.version == '14'

- name: install rust target for MacOS 13 (Intel Mac)
run: rustup target add x86_64-apple-darwin aarch64-apple-darwin
if: matrix.os.base == 'macos' && matrix.os.version == '13'

- name: Update minimum supported MacOS for Wheels for MacOS 13 (Intel Mac)
run: echo "MACOSX_DEPLOYMENT_TARGET=10.12" >> $GITHUB_ENV
if: matrix.os.base == 'macos' && matrix.os.version == '13'

- name: run cargo tests
run: cargo test

- name: set up python
uses: actions/setup-python@v5
uses: actions/setup-python@v4
# uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
python-version: "3.10"

- name: install python dependencies
run: python -m pip install --upgrade pip setuptools wheel pytest
- name: run cargo tests
run: cargo test
run: python -m pip install --upgrade pip setuptools wheel twine cibuildwheel pytest

- name: build source distribution
if: matrix.os.base == 'ubuntu' && matrix.python-version == '10'
run: |
python -m pip install -U setuptools-rust
python -c "import setuptools; setuptools.setup()" sdist

- name: install FASTSim python
run: pip install -e .[dev]
run: python -m pip install -e .[dev]

- name: run pytest
run: pytest -v

- name: build ${{ matrix.platform || matrix.os.base }} binaries
run: cibuildwheel --output-dir dist
env:
CIBW_BUILD: "cp3${{ matrix.python-version }}-*"
CIBW_SKIP: "*-win32 *-musllinux* *i686 *ppc64le *s390x *aarch64"
CIBW_PLATFORM: ${{ matrix.platform || matrix.os.base }}
# TODO: why doesn't pytest work with cibuildwheel?
# CIBW_TEST_COMMAND: "pytest -v {project}/python/fastsim/tests"
CIBW_TEST_COMMAND: "python -m unittest discover {project}/python/fastsim/tests"
CIBW_ARCHS_MACOS: "${{ matrix.os.arch }}"
# see https://cibuildwheel.readthedocs.io/en/stable/faq/#universal2
CIBW_TEST_SKIP: "*_universal2:arm64"
CIBW_ENVIRONMENT: 'PATH="$HOME/.cargo/bin:$PATH"'
CIBW_ENVIRONMENT_WINDOWS: 'PATH="$UserProfile\.cargo\bin;$PATH"'
CIBW_MANYLINUX_X86_64_IMAGE: "manylinux2014"
CIBW_MANYLINUX_I686_IMAGE: "manylinux2014"
CIBW_BEFORE_BUILD: >
pip install -U setuptools-rust &&
rustup default stable &&
rustup show
CIBW_BEFORE_BUILD_LINUX: >
yum -y install openssl openssl-devel &&
pip install -U setuptools-rust &&
curl https://sh.rustup.rs -sSf | sh -s -- --default-toolchain=nightly --profile=minimal -y &&
rustup show

- name: list dist files
run: ${{ matrix.ls || 'ls -lh' }} dist/

- uses: actions/upload-artifact@v4
with:
name: fastsim-${{ matrix.platform || matrix.os.base }}-${{ matrix.os.version }}-py3.${{ matrix.python }}-${{ strategy.job-index }}
path: ./dist/*

2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ authors = [{ name = "NREL/MTES/CIMS/MBAP Group", email = "[email protected]" }]
description = "Tool for modeling vehicle powertrains"
readme = "README.md"
license = { file = "LICENSE.md" }
requires-python = ">=3.8,<3.11"
requires-python = ">=3.10,<3.12"
classifiers = [
"Programming Language :: Python :: 3",
"License :: Other/Proprietary License",
Expand Down
Loading
Loading