-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Run wheel build on PRs #3867
Run wheel build on PRs #3867
Conversation
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.
Great idea. Maybe on merge to main only we run this ... but either way this is handy to know we work and I'll be stealing it for other projects.
I'll trust you to fix CI before merge. I guess running on PR is handy there ...
It's actually not that slow, so we might be able to afford it on PRs. However, it's currently failing and I haven't been able to reproduce that failure mode locally, so I'm going to do some debugging-by-CI here. |
try the mypyc_attr serialisable thing? |
Sorry didn't see this. I tried moving to a plain tuple, will see if that works and try |
This reverts commit bf18575.
This reverts commit 77c312e.
for more information, see https://pre-commit.ci
serializable=True doesn't help, and my attempt to use non-named tuples broke the unit tests for caching. |
Looking back at the release builds from last night, the I have tried and failed to reproduce that issue: when I compile a simple file containing a NamedTuple with mypyc locally, the module is set correctly. |
cc @cdce8p (since you contributed the FileData change) |
Just saw it a few moments ago. Anything I can do to help? I could try to see if I'm able to fix it. |
Would be great if you could take a look! I have seen a bunch of different failures and I don't have any real idea of how to fix them for now:
Using mypy 1.3.0 (which black was using until yesterday) or 1.5.1 doesn't seem to make a difference. |
I managed to find a minimal repro for this one locally using Python 3.12. Filed this mypy issue: |
It's somewhat surprising that the Will need to take another look at it tomorrow. |
This was interesting 😅 Quite a few surprising issues but I mostly got it working now, except for Python 3.12. As for the specific issues:
Open issues (Python 3.12):
Although nice, I don't think it's necessary to ship wheels for Python 3.12 just yet. The sdist already works. Full cibuildwheel Python 3.12 testing log
Edit |
Thanks, that's really helpful! Good catch on the NamedTuple thing being an issue only in 3.8. I wonder if it's worth always pickling the raw tuples, as it makes the file size a lot smaller:
Though it gets amortized a bit if there are many objects:
I thought this might be costly because we'd have to convert to
Presumably pickle is faster at unpickling tuples than NamedTuples. For the |
There is no specific need to use Although, a normal tuple will certainly make it easier to manually unpickle these files if one should ever want to inspect it. |
Thank you @cdce8p! |
So we don't just test this workflow on releases. Maybe every PR is too much, but I want it at least on PRs that prepare releases.