-
Notifications
You must be signed in to change notification settings - Fork 139
Add scipy-stubs
as development depedency
#1598
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
Looks great to me, thanks for the tip! |
Should I fix the new mypy errors this PR, or ignore them for now, so that they can be addressed in a follow-up? Several (maybe even most) of them that don't seem to be related to |
I suspect they are indeed related to the changes |
@jorenham, only a subset of files are currently maintained with passing mypy. See at the bottom of the logs:
Would you be able to make a best-effort attempt to clean up the failing annotations in just these indicated files, ignoring failures in other files? The good news is that a large number of the relevant failures are just unused ignores, thanks to the now-improved typing. |
Yea sure no problem. It's just that I wonder whether some of these aren't also occurring on main at the moment. Because there's quite a bit of overlap between them when I run the mypy script locally on main, where I see 8 errors. |
Indeed!!! I can reproduce locally:
@ricardoV94, assuming that this is due to an uncontrolled change of version of some package, this is the type of problem that lockfiles (#1542) would solve. |
Ah, I think I see what's going on. Due to the way we've defined the caching key, this PR busts the package cache because it changes the environment files. That's why #1599 passes. If I add a comment to the |
"There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors." |
The |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (45.16%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1598 +/- ##
==========================================
- Coverage 81.71% 81.70% -0.02%
==========================================
Files 230 230
Lines 52935 52950 +15
Branches 9404 9404
==========================================
+ Hits 43257 43262 +5
- Misses 7246 7256 +10
Partials 2432 2432
🚀 New features to boost your workflow:
|
Haha sure; rebased |
Excellent. Now that #1599 is in, the rebase was justified. This is now ready to go. |
Thanks!!! |
I'm guessing that the test failure is unrelated? |
Looks like it might be related to #1602 I'll let you guess what is my suggested mitigation this time... 😂 |
58a2aea
to
3ffe552
Compare
3ffe552
to
d84e12e
Compare
@maresb this is good to merge now? |
@jessegrabowski, yes! |
@jorenham thanks for this, you're on an epic streak across the repo :D |
Description
I noticed that
scipy
is used quite a bit and that you're using mypy. SciPy doesn't support type-checking on its own, so you'll need scipy-stubs to get the full static typing benefits, such as improved IDE support (e.g. import autocompletion and inline documentation).I'm it to the conda deps, which I assume is what you use for development.
I'm afraid I wasn't able to locally run the mypy script (using
uv
(because I'm a conda noob) on the main branch it reports that 8 files "unexpectedly failed"). So I can't guarantee that the CI will pass in one go 😅.Related Issue
Checklist
Type of change
📚 Documentation preview 📚: https://pytensor--1598.org.readthedocs.build/en/1598/