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 building and testing issues #467

Merged
merged 32 commits into from
Jan 30, 2025
Merged

Fix building and testing issues #467

merged 32 commits into from
Jan 30, 2025

Conversation

kbonney
Copy link
Collaborator

@kbonney kbonney commented Jan 21, 2025

Summary

This PR addresses several issues with the building and testing process for WNTR:

  • wntr 1.3.0 wheels do not include the epanet binaries. This is the cause of Colab problem #466.

    • This is addressed by correctly referencing the binaries in MANIFEST.in.
  • during the install_import step of the build job, only a basic import wntr test is run on the installed wheel. This causes us to miss deeper problems with the wheel such as the missing binaries mentioned above.

    • This is addressed by testing the wntr test suite after installing the wheels.
  • The wheel for macos-13 is saved with the wrong name (references macos version 14.0 instead of 13.0) and cannot be found when installing from wheels on that OS.

    • This is addressed by forcing the MACOSX_DEPLOYMENT_TARGET environment variable to "13.0" when building for macos-13, which ensures the correct version of macos is targetted. However, the naming issue persists even with this update so the platform name is forced when running the build command.
  • Plotly introduced breaking changes in the latest major version (6.0.0) which affect plot_interactive_network and causes tests to fail. I believe this is the relevant change: Drop support for passing a string to the title attribute, and drop support for deprecated attributes titlefont, titleposition, titleside, and titleoffset plotly/plotly.js#7212.

    • This is addressed by placing an upper bound in requirements.txt to avoid installing the latest version. Future updates, either in this PR or a separate one, can address the breaking change so that the upper bound is unnecessary.

Tests and documentation

n/a

Acknowledgement

By contributing to this software project, I acknowledge that I have reviewed the software quality assurance guidelines and that my contributions are submitted under the Revised BSD License.

@kbonney kbonney mentioned this pull request Jan 21, 2025
@coveralls
Copy link

coveralls commented Jan 21, 2025

Coverage Status

coverage: 81.81%. remained the same
when pulling 6c6ecd2 on kbonney:manifest
into 719e015 on USEPA:main.

@kbonney kbonney changed the title Update manifest to include new binaries Fix building and testing issues Jan 29, 2025
@kaklise kaklise merged commit 2665d4c into USEPA:main Jan 30, 2025
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants