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

Conversation

michael-okeefe
Copy link
Collaborator

Fixes some issues encountered during running pytest -v on Windows:

  • updates how Python is called for demo tests so that it works within a virtual environment
  • Handles how Polars DataFrame is printed by first transforming to a Pandas DataFrame

Additionally, this version adds a CI workflow file to build and test the application.

@michael-okeefe michael-okeefe added the bug Something isn't working label Nov 14, 2024
@michael-okeefe michael-okeefe self-assigned this Nov 14, 2024
@michael-okeefe michael-okeefe marked this pull request as ready for review November 14, 2024 22:18
@michael-okeefe michael-okeefe marked this pull request as draft November 14, 2024 22:19
@michael-okeefe michael-okeefe marked this pull request as ready for review November 14, 2024 23:19
Michael O'Keefe added 4 commits November 15, 2024 11:44
This updates the `build_and_test.yaml` file to also
build source distributions (sdist), wheel files, and
to also save artifacts of those builds.
We updated the python versions to be the full version
(e.g., 3.8, 3.9, or 3.10) versus just the minor version
(e.g., 8, 9, 10). However, the string replacement code
was not updated appropriately that got copied over for
cibuildwheel. This has been corrected.
@michael-okeefe michael-okeefe marked this pull request as draft November 15, 2024 22:04
@michael-okeefe michael-okeefe marked this pull request as ready for review November 22, 2024 17:09
Michael O'Keefe and others added 6 commits November 22, 2024 15:38
Python 3.10 and 3.11 are being tested on the CI.
This bumps the minimum range up to 3.10 from 3.8.

Note: as of today, 3.8 is end-of-life.
Try setting the MACOS_DEPLOYMENT_TARGET equal to 10.12 per instructions from failed pipeline.
- 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.

@calbaker
Copy link
Collaborator

calbaker commented Dec 9, 2024

Handles how Polars DataFrame is printed by first transforming to a Pandas DataFrame
@michael-okeefe , where did this change happen? I'm struggling to find it.

@michael-okeefe
Copy link
Collaborator Author

Handles how Polars DataFrame is printed by first transforming to a Pandas DataFrame
@michael-okeefe , where did this change happen? I'm struggling to find it.

@calbaker Apologies, I must have gotten my PRs mixed up as I don't see the change here, either. This was the area where the issue occurred during tests:

https://github.com/NREL/fastsim/blob/f3/fix-pytest-issues/python/fastsim/demos/demo_variable_paths.py#L49

Not sure if this is the exact line or not, though.

The Mac OS options might be hard for others to follow. Here, we add a note to help others following behind to understand what we're doing and why.
@@ -7,6 +7,9 @@ jobs:
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 14 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 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.

@calbaker calbaker merged commit fcde5a1 into fastsim-3 Dec 10, 2024
8 checks passed
@calbaker calbaker deleted the f3/fix-pytest-issues branch December 10, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants