Skip to content

Add test matrix against operating environments #821

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

Merged
merged 5 commits into from
Dec 24, 2022

Conversation

murrayrm
Copy link
Member

@murrayrm murrayrm commented Dec 24, 2022

This PR adds a GitHub action to run unit tests against a (large) matrix of different environments, including operating system, BLAS variant, and installation/build method (pip, conda), copied over from slycot. This is in response to issue #820, where an error was triggered under Ubuntu using the generic BLAS library for NumPy 1.24.0.

The matrix of tests is quite time consuming (about 45 minutes), and so the action is only trigged on pull requests (so if you are using GitHub actions in your own forks, they won't be affected).

In the initial PR, the errors from issue #820 are present (so the tests will fail). In a subsequent push to this PR, I added a "fix" by doing an xfail on that specific test case (and demonstrating one way this set of test might be used).

Also: when a failure occurs in the flatsys test case, information is printed out about the configuration to help in identifying the cases that are failing.

@murrayrm murrayrm force-pushed the test_matrix-22Dec2022 branch from f246732 to f477c13 Compare December 24, 2022 04:00
@murrayrm murrayrm linked an issue Dec 24, 2022 that may be closed by this pull request
@coveralls
Copy link

coveralls commented Dec 24, 2022

Coverage Status

Coverage: 94.803%. Remained the same when pulling 4d6a6bf on murrayrm:test_matrix-22Dec2022 into 56f227f on python-control:main.

@murrayrm
Copy link
Member Author

Added checks + xfail for Ubuntu with generic BLAs with numpy 1.24.0, in response to issue #820. Near as I can tell, this is some sort of edge case that gets triggered under this specific set of circumstances. Not sure this is the right way to handle it, but given that all of the other environments work out, I don't think it is a bug in solve_flat_ocp.

@bnavigator
Copy link
Contributor

python-control is pure Python and should not depend on the environment. Problems in the environment usually show up when the consumed libraries change, not during python-control PRs.

We already go through that test matrix in the Slycot CI and run the python-control test suite there. Not sure if we really should duplicate the resource consumption here.

@murrayrm
Copy link
Member Author

murrayrm commented Dec 24, 2022

I agree this shouldn't be needed @bnavigator, but the failure pointed out in issue #820 is an example of where python-control does depend on the operating environment, because there is some sort of (non-slycot) interaction with something in the build environment (and only under Ubuntu, NumPy 1.24, generic BLAS).

I've changed the trigger to workflow_dispatch, which allows this to be manually triggered on the main branch, plus on PRs that affect files specifically associated with this workflow. That will get rid of the resource consumption except in the rather exceptional cases where we need it. (I also imagine that we might customize the test matrix here over time, so it could diverge from slycot.)

My current plan is to add a step in the release Instructions suggesting running this workflow before a release, just to flag anything where python-control depends on the details of the OS, BLAS version, or installation method.

@murrayrm murrayrm merged commit f15fc0f into python-control:main Dec 24, 2022
@murrayrm murrayrm added this to the 0.9.3 milestone Dec 29, 2022
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.

Flaky test failure in TestFlatSys with current slycot
3 participants