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

Cache PROJ library build when building wheels #1345

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

djhoese
Copy link
Contributor

@djhoese djhoese commented Sep 22, 2023

As discussed in #1342 PROJ building takes about 10 minutes at the top of every wheel build workflow. This PR caches it with GitHub Actions to hopefully remove this requirement except for when PROJ (or its dependencies) are updated.

This workflow is taken from shapely's own CI.

  • Closes #xxxx
  • Tests added
  • Fully documented, including history.rst for all changes and api/*.rst for new API

@djhoese djhoese added the dependencies Pull requests that update a dependency file label Sep 22, 2023
@djhoese djhoese self-assigned this Sep 22, 2023
@djhoese
Copy link
Contributor Author

djhoese commented Sep 22, 2023

I swear I write enough bash to not have made the mistakes I made in this PR. I don't think my coffee kicked in yet.

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Merging #1345 (a4aba7e) into main (3cea713) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1345   +/-   ##
=======================================
  Coverage   96.41%   96.41%           
=======================================
  Files          20       20           
  Lines        1812     1812           
=======================================
  Hits         1747     1747           
  Misses         65       65           

@djhoese
Copy link
Contributor Author

djhoese commented Sep 22, 2023

Damn I was so sure I had this working. It even restored the directory...but then it didn't use it.

@djhoese
Copy link
Contributor Author

djhoese commented Sep 22, 2023

Not sure why but throughout my testing the PROJ building has been improving in time from ~600s to ~480s. Maybe some download caching inside github? Not really sure.

Watching the current builds it seems like the cache is working.

@djhoese
Copy link
Contributor Author

djhoese commented Sep 22, 2023

...and nevermind it needs libtiff and sqlite too. I was only caching the PROJ directory.

@djhoese
Copy link
Contributor Author

djhoese commented Sep 24, 2023

@snowman2 FYI this works now, but has a lot of debug and ugly ways of doing things just as a proof of concept.

I'm very surprised at the variance in the build times both of PROJ itself and the pyproj wheel building. They seems to very by quite a bit. I've seen PROJ go between ~4 minutes to ~6 minutes through my testing.

One of the biggest surprises but one that still makes sense was that caching the libtiff and other PROJ deps in the cache directory meant I had to put it in LD_LIBRARY_PATH, but only on the linux system. Otherwise, auditwheel was failing to find them. One solution I thought of was to add the cache directory to LD_RUN_PATH versus depending on LD_LIBRARY_PATH so that I didn't need to carry aroundthis extra library path (it would be in the PROJ .so). My understanding is that auditwheel should bundle all the depended on libraries anyway so this shouldn't really matter. Does any of this concern you? Or do you have other things you'd like to see as I work on cleaning this code up?

@snowman2
Copy link
Member

Thanks @djhoese 👍 - I am looking forward to this update. I am busy this week, but should have a moment next week to take a look.

This should mean that the built .so has a reference to the path and the user isn't required to add it
@djhoese djhoese marked this pull request as ready for review September 26, 2023 15:09
@snowman2
Copy link
Member

snowman2 commented Oct 2, 2023

No concerns from my end. This change makes sense to me. Thank you for working on this 👍

@djhoese
Copy link
Contributor Author

djhoese commented Oct 3, 2023

I just wish it was a bigger win for build times than it is.

@snowman2 snowman2 force-pushed the main branch 6 times, most recently from 36d5482 to 7da1053 Compare August 8, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants