Skip to content
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

Add cibuildwheel #537

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Add cibuildwheel #537

wants to merge 3 commits into from

Conversation

ocefpaf
Copy link

@ocefpaf ocefpaf commented Sep 7, 2024

@rossbar this is not ready yet. We are missing:

However, it is building and creating wheels+sdist and this GHA can upload to releases PyPI if you configure a token for it.
The artifacts can also be downloaded and tested. These are the libs that are getting into the wheel at the moment:

macos:

40K libcdt.5.dylib*
115K libcgraph.6.dylib*
184K libexpat.1.9.3.dylib*
614K libgvc.6.dylib*
57K libpathplan.4.dylib*
49K libxdot.4.dylib*
118K libz.1.3.1.dylib*

Windows:

22K cdt-342f69fc3474f83e64f7adc41ab41c06.dll
101K cgraph-0a3e783e69a7584e6aab91066c42041e.dll
586K gvc-8a9c330349805b089cded3d80a6df4c7.dll
393K libexpat-240eda6cb38cafdca500c9bac45c9e1e.dll
40K pathplan-15678f6e0d92308f4acb1bbfb5ccc80d.dll
27K xdot-2e38ffa2a89bc2ee943b244c5295e4c0.dll
87K zlib-bcc560fd7c0e05149614f4743cc3939c.dll

I don't see cairo in there, so there is still some work to do on that front.

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incredible @ocefpaf , I can't thank you enough for taking the time to get us started here!

The cibuildwheel infrastructure is one of the two necessary ingredients to resolve pygraphviz 's number one issue (i.e. installation) once and for all! I'm inclined to merge now even though this isn't 100% finished so that we can use this to test/evaluate the other necessary ingredient in #421 (i.e. fully severing the ties to the graphviz CLI).

Does that sound okay on your front? If there are any other tweaks you'd like to get in this PR specifically I'm happy to wait, but if not I'm inclined to merge and continue iterating in followup PRs. LMK what you think/prefer!

@ocefpaf
Copy link
Author

ocefpaf commented Sep 9, 2024

Does that sound okay on your front? If there are any other tweaks you'd like to get in this PR specifically I'm happy to wait, but if not I'm inclined to merge and continue iterating in followup PRs. LMK what you think/prefer!

It is ultimately up to you but I am working on Linux today and, if I can get it to the same state as macos and Windows, it would be better to merge them together and clean it up later. Can you give me until tomorrow?

@rossbar
Copy link
Contributor

rossbar commented Sep 9, 2024

It is ultimately up to you but I am working on Linux today and, if I can get it to the same state as macos and Windows, it would be better to merge them together and clean it up later. Can you give me until tomorrow?

Absolutely - there's no rush at all! Just ping when you've hit a checkpoint your satisfied with and we'll go from there!

@ocefpaf ocefpaf force-pushed the win_wheels branch 2 times, most recently from 51db538 to 73c9461 Compare September 10, 2024 16:03
@ocefpaf
Copy link
Author

ocefpaf commented Sep 10, 2024

OK, it is a bit cleaner now, consolidated everything in a single job. On Linux we are using the latest manylinux so we can have access to a slightly modern graphviz from the system. Ideally we should build it from source (for all platforms actually), or figure out how to use the conda-forge with cibuildwheel like the Windows and macos setup here.

I've been using this setup for Windows for a while with success, never for macos. I recommend caution, like publishing them as RCs/dev release only to get user feedback. On Linux there are 3 tests, besides the expected CLI ones, that are failing:

  FAILED agraph.py::pygraphviz.agraph.AGraph.add_edge
  FAILED tests/test_graph.py::TestGraph::test_graph_not_strict - AssertionError...
  FAILED tests/test_graph.py::TestDiGraphOnly::test_graph_not_strict - Assertio...

My guess those are related to the old graphviz version available in the systen.

I'm done here. Hopefully the skeleton to build something better is in this PR and it is useful for the next iteration. If you want to invest time in the graphviz build for Linux I recommend creating your own manylinux docker image. It will speed up the process significantly. See https://github.com/ocefpaf/netcdf-manylinux for an example.

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks @ocefpaf . I'm inclined to get this in now so the infrastructure is in place to keep cranking on #421 (and related). Any objections to that course of action @jarrodmillman @dschult @MridulS ?

Again @ocefpaf I can't overstate how helpful this is, so thanks so much for getting this setup!

@jarrodmillman
Copy link
Contributor

@ocefpaf This is awesome! Thanks so much. I am going to release pygraphviz-1.14 this week. I will merge this PR after that release. We will focus on wheels for the post 1.14 release and call the next release 2.0! I am marking as draft for the next few days, so we don't merge this before the 1.14 release.

@jarrodmillman jarrodmillman marked this pull request as draft September 11, 2024 18:37
@ocefpaf
Copy link
Author

ocefpaf commented Sep 11, 2024

so we don't merge this before the 1.14 release.

I'm travelling and probably won't be able to to much here but, as soon as you mint the release, feel free to ping me to update the tests and this PR. That should be easy enough for me to do from any place.

@jarrodmillman jarrodmillman added this to the 2.0 milestone Sep 29, 2024
@jarrodmillman jarrodmillman changed the title add cibuildwheel Add cibuildwheel Sep 30, 2024
@jarrodmillman
Copy link
Contributor

@ocefpaf pygraphviz 1.14 is released! It would be great if you could rebase on main and get this PR ready to merge when you have time. Thanks!!

@ocefpaf
Copy link
Author

ocefpaf commented Oct 1, 2024

@ocefpaf pygraphviz 1.14 is released! It would be great if you could rebase on main and get this PR ready to merge when you have time. Thanks!!

Done!

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.97%. Comparing base (e44e4dd) to head (df295f4).
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #537   +/-   ##
=======================================
  Coverage   80.97%   80.97%           
=======================================
  Files           5        5           
  Lines        1251     1251           
=======================================
  Hits         1013     1013           
  Misses        238      238           
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ocefpaf
Copy link
Author

ocefpaf commented Oct 1, 2024

Looks like it is segfaulting:-(

I'll investigate it tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants