Skip to content

Conversation

tiran
Copy link
Contributor

@tiran tiran commented Apr 20, 2024

Changes

Which issue is resolved by this Pull Request:
See #814

Description of your changes:

Tests now use a pytest fixture to mock-patch the mlx package. This resolves a problem with my old mock_mlx wrapper, which broke testing with Python 3.9 and 3.10

Kudos to Ihar Hrachyshka for discovering the problem and reporting it in issue #814.

@github-actions github-actions bot added the testing Relates to testing label Apr 20, 2024
Tests now use a pytest fixture to mock-patch the mlx package. This
resolves a problem with my old `mock_mlx` wrapper, which broke testing
with Python 3.9 and 3.10

Kudos to Ihar Hrachyshka for discovering the problem and reporting it in
issue instructlab#814.

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@booxter
Copy link
Contributor

booxter commented Apr 22, 2024

Confirming this works. I could (with #962) pass test run on python3.9 / rhel8.4 machine.

Copy link
Contributor

@bjhargrave bjhargrave left a comment

Choose a reason for hiding this comment

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

I can also confirm this PR allows unit tests to pass on python 3.9 on macOS. Thanks!

@bjhargrave
Copy link
Contributor

Perhaps we should now add 3.9 to the python test matrix?

python:
- "3.11"

@tiran
Copy link
Contributor Author

tiran commented Apr 23, 2024

Perhaps we should now add 3.9 to the python test matrix?

My PR #954 makes it easier to test with multiple Python versions. Once the PR has landed, you can do py39-unit to run unit tests with Python 3.9.

@bjhargrave
Copy link
Contributor

Once the PR has landed, you can do py39-unit to run unit tests with Python 3.9.

That is very useful for local testing if developers think to do it. But I still think that if we claim to support python 3.9, then our ci build needs to confirm that via regular testing. One could add code which runs fine on 3.11 but not on 3.9 and we may not find this until much later than we could have known if the ci build tests on 3.9.

@tiran
Copy link
Contributor Author

tiran commented Apr 23, 2024

That is very useful for local testing if developers think to do it. But I still think that if we claim to support python 3.9, then our ci build needs to confirm that via regular testing. One could add code which runs fine on 3.11 but not on 3.9 and we may not find this until much later than we could have known if the ci build tests on 3.9.

You also need the change for CI to run tests with multiple Python versions. A common approach is tox-gh-action with a Python matrix and mapping of matrix env to tox env. The project documentation for tox-gh-action has documentation how to set up a test matrix.

@russellb russellb merged commit b81dcad into instructlab:main Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Relates to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants