-
Notifications
You must be signed in to change notification settings - Fork 460
Run and Cache Regressions on Develop And reuse in PR workflows #11190
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
Conversation
62 commits were squashed
Try caching and co
copy paste issue
Gotta checkout baseline so I find the composite action
path issue
Missing shell/uses
Wrong path
Botched the cache key
Ccache: cache key with COMPILER version (future proofing GHA updating the compilers)
setup-msbuild does not expose cl.exe and co, so do it manually
Wrong target
Wrong order for GIT SHA used in cache key
Use runner.os not matrix.pretty in cache key
Dumb change to see caching in action
Show ccache -p. And try to see if that speeds up the build (no code change).
It took ALSO 8 min to build E+ with a dumb simple change even though it restored ccache
is this because of PCH?
Try to disable PCH to see if ccache works without
Dumb change to see if ccache works without PCH now...
bothced N => NPROC replacement
I'm getting a weird error, force git sha to 10 characters
Put back problem matchers
Try to avoid a checkout of baseline if not needed, build ENTIRE E+ on develop (caching), configure CCACHE_BASEDIR. See notes
* Disabled PCH (and deleted the actions' cache): build took 10 min, not 8min.
* Made another change to Boilers.cc and rebuilt: this time the build took ONE (1) minute.
* Made a PR:
* The baseline regression tests are picked up correctly, and baseline isn't rebuilt. That's good.
* The CCache thing isn't working. And because of NO PCH the build takes 20 minutes instead of 14
Notes:
* On develop, I suppose you still want to build the entire project, not just energyplus + some fortran stuff, BECAUSE you're doing that in test_pull_requests, and you want a primed CCache folder when you do
* CCache will not hit a cache if you build at another path, because CMake puts a bunch of absolute paths. So there's two ways to fix that:
* Build at the SAME path.
* Try to configure CCache to ignore that, via base_dir(CCACHE_BASEDIR)
TEMPORARY( revert!) just run a couple of integration tests
Try to use action directly from github
Ok whatever, just be too damn clever and move on
Is a composite action a good idea? Really?
COmposite action doesn't access secrets, just use https
Bash typo
Dunno where I messed up STORING the sha
Start another version that uses composite steps differently, maybe it's more readable
Botched paths to actions
Gotta map the value of the composite action outputs explicitly!
typo
Wrong arg
typo
Typo in bash
Tweaks
PR: should disable PCH too.
ON PR, cache ccache only by branch name I guess. Consider just not caching perhaps.
adjust the unpacked PR paths
When a PR is built without the reg tests found from develop (race condition or caching issue), perhaps adding PCH is better
Ubuntu; try with PCH to see if that works. Set slopiness to pch_defines,time_macros
https://ccache.dev/manual/latest.html#_precompiled_headers
Would a subsequent develop build work with PCH?
default ccache base dir to the workspace
Use Pwsh on Windows, use my "unpacked version" (I find it clearer), and enable all platforms. Turning off PCH for now
Windows quote issue
Try arm-64 versions for Ubuntu 24.04 and Windows 11
Gotta change the cache key so that Linux arm64 doesn't pick up cache from the x64 one
Tweak: use a listed Python arm64 for windows + a quote issue
pwsh doesn't respect exit codes apparently
actions/runner-images#6668
Allow building zlib on windows arm64
make it -VV to see why 1ZoneUncontrolledUTF8 is failing on ubuntu arm
pwsh quote issue
Forgot to call target_architecture
Disable the UTF8 test that fails on ubuntu arm
Aparently I need tzdata explicitly on windows
Add a workflow to clean caches from merged branches
cf https://docs.github.com/en/actions/how-tos/manage-workflow-runs/manage-caches#force-deleting-cache-entries
show ccache config after building for debug
Missing an s! so condition was evaluating to "null" and the baseline wasn't checked out
Avoid wipping the build/ and .ccache dirs
Of course the ReadVarsESO target is called ReadVars on windows. OF COURSE.
fixup ninja command
Clean up and move back the workflows
Put back the problem matchers
|
The Build and Test thing isn't working here because when develop is checked out, the shared action isn't found, since it's not on develop yet. Ignore that |
Myoldmopar
left a comment
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.
@jmarrec this could go down near the top of monumental changes to the E+ build process. I'm incredibly excited about this, and would love to see how we could build on this to improve the regression outputs for incredible reviews. Also I'd like to see if we can now add coverage results to GHA so that we can immediately sunset Decent CI.
I think the best thing to do here is to get this merged right in, and if there are any hiccups, we'll just deal with them after.
| arch: x86_64 | ||
| python-arch: x64 | ||
| run_regressions: true | ||
| pretty: "Windows x64" |
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.
It's really exciting to see the platforms added back in now that CI will be speedy.
Pull request overview
This pull requests enables caching both CCache and regression results from develop.
What does this mean in practice?
A PR workflow with a minimal change (touching a few lines in Boilers.cc) will run under 6 minutes versus about 52 minutes
Description of the purpose of this PR
60+ commits (probably close to a 100 in reality since I squashed and force pushed a bunch) were eventually squashed into one. Some details can be found on my fork at https://github.com/jmarrec/EnergyPlus/compare/develop_cache_details...jmarrec:EnergyPlus:cache_regression_details?expand=1
This PR does several things, some of which can be turned off:
setup-runner: takes care of installing the system dependencies such as ninja and ccache, and configuring ccache, (and figuring out nproc)configure-and-build: takes care of calling CMake Configure with the appropriate options regardless of whether we use windows or Unix, then buildings with ninja (with minimal targets for integration only or the full thing, based on a boolean)What testing has been conducted?
Well, I've done a LOT of runs on my fork.
After squashing all these commits, here is a run after I did
gh cache delete --allso there are zero ccaches to be found:Some caveat on precompiled headers:
Future investigations into re-enabling PCH should start with this
Pull Request Author
Reviewer