Conversation
mikeurbach
left a comment
There was a problem hiding this comment.
This looks good to me, just one small suggestion. I was hoping in the github actions we could use an if: predicate to skip the step that actually uploads wheels, but I see now that it uploads the whole batch in one step. Removing the macos wheels from the local directory before upload seem fine, not sure if there is a better way without broader changes to this job.
The other thing I want to be clear about: @jumerckx will you be ok to maintain this CI/CD flow? If it breaks, and that is causing issues during releases, etc., we will either need you to fix it or else we will probably end up removing it again.
| matrix="{ | ||
| \"config\": [ | ||
| {\"os\":\"$os\",\"cibw_build\":\"$cibw_build\"} | ||
| ] | ||
| }' | ||
| }" |
There was a problem hiding this comment.
nit: I would write this as it was before a) for consistency with the other case, and b) to reduce line noise in the diff. It's also nice to not have to escape so many double quotes.
| matrix="{ | |
| \"config\": [ | |
| {\"os\":\"$os\",\"cibw_build\":\"$cibw_build\"} | |
| ] | |
| }' | |
| }" | |
| matrix='{ | |
| "config": [ | |
| {"os":"${{ os }}","cibw_build":"${{ cibw_build }}"} | |
| ] | |
| }' |
BTW, since this mentioned being agent-assisted, I include the following from the top of LLVM's coding standards verbatim in my agent rules:
If you are extending, enhancing, or bug fixing already implemented code, use the style that is already being used so that the source is uniform and easy to follow.
That helps avoid these situations where the if and else branches use a different style.
|
I'm generally good with publishing these to PyPi as long as we have the space. Or: building these may be of questionable utility if they aren't published to PyPi. The primary issue why this was disabled looks like it was because it was broken and no one was maintaining it: #8051 I'm not sure how to balance this with the space issue other than pruning all the weekly/manual development releases for testing. |
|
Thanks a lot for your reviews! I've now also reverted the change to exclude macos from pypi.
Thanks for bringing this up. I'm fine with maintaining macos CI/CD for the foreseeable future, under the assumption that it doesn't break for every commit. |
|
Thanks! This looks good to me assumed that release CI passes. IMO if the space is concerning we should stop weekly nightly publication to pypi, or at least stop publishing wheels for different versions. SiFive's internal flow works fine with wheel uploaded from github action. (BTW I’m a big fan of Tamagoyaki/Rover! Thank you for the interesting work! Maybe CIRCT (especially comb/datapath) should also utilize pdl more.) |
|
@uenoku - there's actually been some work to convert some of comb's canonicalizations to pdl ("First-Class Verification Dialects for MLIR") - and with Adrian Sampson at Cornell we've started at looking at a slightly less verbose and easier to verify DSL that compiles to PDL That way we can get Alive style proofs that new comb/datapath transformations are correct :) |
|
My only hang up at this point is that yes, space on PyPI is a concern, and as an abstract principle I really hate deleting artifacts from PyPI. I've done it for CIRCT, I'm just trying to avoid making a habit of it :) For context, it was a ~months turnaround to get them to update us from the default 10GB to 20GB, and we are currently using 15.4GB. Each release is currently ~111MB from three 37MB wheels for different versions. I like @uenoku's suggestion to stop pushing the dev wheels weekly, that way we just get wheels when we cut a periodic release. That seems like a fair tradeoff to bring on a couple more wheels per build. We can always manually trigger a dev CI/CD run with wheel upload enabled if necessary as well. I'll just go ahead and push a commit to remove that, and I think we can merge this whenever you're ready @jumerckx . Did you trigger the Upload Wheels CI job on this branch? Also, I second what Hideto said, your projects are awesome and we're always interested in collaborations. |
|
@seldridge does that work for your release workflow? Are you using the weekly dev wheel at all when you qualify a release? |
That's fantastic! |
|
Commented on the other PR. No, not using it. I usually build on demand if there are failures and have been YOLO-ing otherwise (if the qualification passes, I release and then assume the Python wheels work and deal with the fallback when it doesn't). |
While discussing #10371, we considered removing the weekly dev wheel builds to save space on PyPI. I think that is a great idea, so I'm just removing it here. We can always add it back if needed, but I think they don't really serve much purpose today anyway.
@mikeurbach told me on discord that macos wheels were disabled because PyPI storage ran out, and there's little interest from others. This pr re-enables generation of wheels for macos, but shouldn't push them to PyPI.
I tested things a little on my fork: jumerckx#2 (there are also some changes to esiruntime before I realized I only want the circt wheels.)
For now the wheels for macos are only built for arm macs with relatively recent python versions (>= 3.13).
Assisted-by: AMP