-
Notifications
You must be signed in to change notification settings - Fork 16
Add unit tests #7
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
Conversation
| python_requires = >=3.8 | ||
| install_requires = | ||
| numpy>=1.7.0 | ||
| numpy>=1.18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump to lowest tested versions
python 3.8 is the lowest available for actions/setup-python
numpy 1.18 is the lowest available for python 3.8
87463b8 to
9fe1fff
Compare
ngoldbaum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just left a few comments.
|
|
||
| # FIXME warning on NumPy 2.4 when downloading pre-computed pickles: | ||
| # Python or NumPy boolean but got `align=0`. | ||
| # Did you mean to pass a tuple to create a subarray type? (Deprecated NumPy 2.4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just re-saving the pickle files with numpy 2.4 will fix this. But then you'd need to replace the files in the AWS bucket...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. I'd open a follow-up ticket but the issues tab has been disabled for this repo.
| # TODO test output contents | ||
|
|
||
|
|
||
| @pytest.mark.skip(reason="very slow download") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's always going to be skipped, why add the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To inform whoever reads it next that today the whole feature is functionally unusable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make that clear in the message or with a FIXME comment?
This PR was initiated by downstream failures in thinc CI with NumPy 2.4.
cifarandwikinerUnsure why CI isn't running. A succesful CI run is visible at crusaderky#1