Conversation
|
Ehh, are the test failures (segfaults) caused by these changes...? |
Can you pass the tests locally? |
Ah your intuition appears to be right; I can't. I'll try and figure out where I messed up then, and marking this as draft in the meantime |
|
The tests are passing locally now; sorry for the noise |
FrancescAlted
left a comment
There was a problem hiding this comment.
Great work @jorenham ! However, I am not sure which improvements this represents to the users, as they will mainly use numexpr.evaluate(), which has a quite easy signature. Perhaps this is more meant to developers?
Also, I would not touch the cpuinfo.py module, which is vendored from the https://github.com/workhorsy/py-cpuinfo project.
Yea a bit of both I guess. As a user I often tend to look at the source of libraries I use, e.g. if the docs aren't sufficiently clear. Type annotations can help a lot with understanding the codebase. Typing only a part of a codebase can get pretty messy too, as that would involve either having to litter The public api will always rely on the private api in some way. And if type-checkers don't know how to infer some type, it'll be marked as an error (at least when run in strict mode, like we're doing here). So that's why in practice I think that it's often easier to just annotate everything. It's a bit of work, but once you've covered everything, then the maintenance burden is minimal in mature projects like this one.
I guess I'll be submitting a PR there too then :) Should I remove here for the time being? Or do we keep it in here, assuming that |
Looking at the |
FrancescAlted
left a comment
There was a problem hiding this comment.
LGTM. Thanks @jorenham !
Ok. As it seems like the py-cpuinfo is more than 3 years without receiving updates, I suppose that a new PR on typing won't be accepted immediately, so I have accepted your changes here. But it would be nice if you propose these to the upstream py-cpuinfo project too. Thanks! |
Ups, probably numexpr needs to update the vendored py-cpuinfo. Hopefully, the changes are not too backward incompatible. |
FWIW, I tried my best to minimize the runtime changes, so assuming I succeeded, it shouldn't be a problem |
|
Hello, unfortunately we had to rollback this PR due to unavailabililty of numpy.typing in numpy<2.0. It seems that numpy._typing is available in e.g. numpy 1.26 (which we support). One possibility is to modify this PR to do something like as a fix (which I'm not sure I am happy with tbh). Or we could wait until it is reasonable to phase out support for numpy <2.0. |
|
|
Hi yes I am aware of this. However for some reason |
With this, NumExpr will now play nicely with type-checkers such as mypy and pyright. But to be honest, I wasn't expecting this much code to be hiding under the surface, so it turned out to be "a bit" more effort then I had initially anticipated haha
.
There have been several cases where I touched up some syntax, but I tried my best to avoid changing any of the current behavior. I added some seemingly redundant assertions and some
if ...guards to ensure that mypy/pyright apply type-narrowing, i.e. infer some type asTinstead ofT | None. In those cases the alternative would be to throw around a bunch of# type: ignorecomments, but with those we'd also risk hiding the actual type-errors.Anyway, let me know what you think; no need to hold back ;)