Skip to content

Cache PROJ library build when building wheels#1345

Open
djhoese wants to merge 20 commits into
pyproj4:mainfrom
djhoese:ci-opt-wheel
Open

Cache PROJ library build when building wheels#1345
djhoese wants to merge 20 commits into
pyproj4:mainfrom
djhoese:ci-opt-wheel

Conversation

@djhoese

@djhoese djhoese commented Sep 22, 2023

Copy link
Copy Markdown
Contributor

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 requested a review from snowman2 September 22, 2023 14:38
@djhoese djhoese self-assigned this Sep 22, 2023
@djhoese

djhoese commented Sep 22, 2023

Copy link
Copy Markdown
Contributor Author

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

codecov Bot commented Sep 22, 2023

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.41%. Comparing base (3cea713) to head (a4aba7e).
⚠️ Report is 169 commits behind head on main.

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@djhoese

djhoese commented Sep 22, 2023

Copy link
Copy Markdown
Contributor Author

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

@djhoese

djhoese commented Sep 22, 2023

Copy link
Copy Markdown
Contributor Author

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

djhoese commented Sep 22, 2023

Copy link
Copy Markdown
Contributor Author

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

@djhoese

djhoese commented Sep 24, 2023

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

snowman2 commented Oct 2, 2023

Copy link
Copy Markdown
Member

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

@djhoese

djhoese commented Oct 3, 2023

Copy link
Copy Markdown
Contributor Author

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