-
Notifications
You must be signed in to change notification settings - Fork 160
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
JDBetteridge/update caching #3730
Conversation
d4c09f8
to
13c8592
Compare
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.
I've made a couple of comments but I think I need talking through this.
# Drop prefix as it's only used for naming and log | ||
# JBTODO: Can't drop prefix as tests/slate/test_optimise.py::test_partially_optimised fails, investigate | ||
# it looks like the prefix is being used to create different subkernels, which conflicts with the docstring below | ||
return default_parallel_hashkey(form.signature(), prefix, parameters, interface, diagonal) |
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.
not sure I understand the change. Why are we allowed to drop coefficient_numbers
?
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.
coefficient_numbers
is actually derived directly from form
so it doesn't need to be included. Also this cache now sits at a slightly different point in the stack so coefficient_numbers
isn't even an argument to the wrapped function.
13c8592
to
6df0e3a
Compare
…e that's how dependencies are supposed to work...
6df0e3a
to
46f2ea1
Compare
|
|
I think this one is ready to go (once tests pass of course!) |
Co-authored-by: Jack Betteridge <[email protected]>
Description
Necessary caches replaced with parallel safe ones.
Depends on PyOP2 #724 going in first.
The PyOP2 caches are used in 3 key places:
tsfc_interface.py
: TSFC no longer inherits from the (removed)Cached
object and thetsfc_compile_form
function is now a cached function.slate/slac/compiler.py
: SlateKernel still inherits from TSFCKernel, thecompile_expression
function is now cachedinterpolation.py
: Thecompile_expression
function is cached with the new decorator.The tests have been updated to run with the new
PYOP2_SPMD_STRICT
environment variable set to 1. Now everything passes without deadlocking, with the exception of two tests utilising ngsPETSc, which requires an upstream fix.Testing performance has been significantly improved, but this isn't apparent on CI (I'm still not 100% sure why). Results are as follows:
Commands
Run on my workstation:
Current Firedrake master
(
export REV="Old"
)cold
Output:
This branch
(
export REV="Experiment"
)cold
Output:
I also ran with a warm cache with similar levels of improvement, but this isn't as relevant (to CI).