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

Problems pickling objects #699

Closed
TomTranter opened this issue Oct 31, 2019 · 23 comments · Fixed by #723
Closed

Problems pickling objects #699

TomTranter opened this issue Oct 31, 2019 · 23 comments · Fixed by #723
Assignees

Comments

@TomTranter
Copy link
Contributor

Describe the bug
Some objects cannot be pickled due to the following error:
AttributeError: Can't pickle local object 'primitive..f_wrapped'

To Reproduce

import pybamm
import numpy as np
import pickle

def save_obj(fname, obj):
    with open(fname, 'wb') as f:
        pickle.dump(obj, f)

pybamm.set_logging_level("INFO")

# load model
model = pybamm.lithium_ion.SPMe()

# create geometry
geometry = model.default_geometry

# load parameter values and process model and geometry
param = model.default_parameter_values
param.process_model(model)
param.process_geometry(geometry)

# set mesh
mesh = pybamm.Mesh(geometry, model.default_submesh_types, model.default_var_pts)

# discretise model
disc = pybamm.Discretisation(mesh, model.default_spatial_methods)
disc.process_model(model)

# solve model
t_eval = np.linspace(0, 0.2, 100)
solver = pybamm.KLU()
solution = solver.solve(model, t_eval)

# plot
plot = pybamm.QuickPlot(model, mesh, solution)
plot.dynamic_plot()

save_obj('test_save_param', param)

Expected behaviour
Pickle saves the object

Additional context
This type of error can occur when classes are defined within other classes or functions i.e. not the global scope. There was one example of this in pybamm with the klu solver but I think the problem may also be with autograd as the f_wrapped function is from there

@TomTranter
Copy link
Contributor Author

Pickling geometry mesh and solution all work
param, model and disc all refer to f_wrapped
solver refers to DaeSolver.set_up in the error

@TomTranter
Copy link
Contributor Author

@TomTranter
Copy link
Contributor Author

@tinosulzer it looks like the problem is definitely with autograd. It seems we use it everywhere though! The above comment references using sympy or casadi. I know a bit about sympy and you seem to know about casadi. What do you suggest we do?

@valentinsulzer
Copy link
Member

Worth checking both sympy and casadi both work with pickling first, but if either one does then I'm happy to drop autograd. It shouldn't be too bad, only appears when related to differentiating functions?

Casadi would definitely work and probably be quicker, but if you're more comfortable implementing sympy and there won't be any performance loss then go for that

@TomTranter
Copy link
Contributor Author

Is your casadi branch ready?

@valentinsulzer
Copy link
Member

valentinsulzer commented Oct 31, 2019

most of the casadi functionality is in master now, just a few small things to wrap up which is what I'm working on now in separate issues

Edit: just checked and you can't pickle casadi either 😦:

import pickle                                                                                                                                                                                                                                                                                                 
import casadi                                                                                                                                                                                                                                                                                                 

a = casadi.SX.sym("a")                                                                                                                                                                                                                                                                                        

with open("file","wb") as f: 
     pickle.dump(a, f) 

gives

TypeError: can't pickle SwigPyObject objects

@TomTranter
Copy link
Contributor Author

So the only thing autograd is used for is to differentiate the interpolated functions is that right?

@valentinsulzer
Copy link
Member

No, it's used to differentiate generic user-specified functions such as the diffusivity or OCP, interpolated functions have their own inbuilt differentiation routines. But this is the only thing it's used for

@TomTranter
Copy link
Contributor Author

Could a solution be to create interpolated functions out of these user-specified ones? Or get the users to specify the derivative too? Sympy could possibly work. There's also a package called jax that might do it but it's linux only

@valentinsulzer
Copy link
Member

Not a fan of getting users to specify derivatives as that could easily get out of sync, creating interpolated functions could work but requires knowing ranges of values. We could just write our own rules for all the different functions though, maybe that's the easiest, I was planning on doing this anyway at some point

@TomTranter
Copy link
Contributor Author

Not sure what you mean by that

@valentinsulzer
Copy link
Member

At the moment we have Symbol.diff, which applies the chain and product rules to expression trees to find derivatives. But we fall back to autograd when we haven't implemented a specific function (exp, log, tanh, etc). We could just implement all of the functions we need and specify their derivatives, and never need autograd. I've already done this for a few, but not all.

@valentinsulzer valentinsulzer self-assigned this Oct 31, 2019
@valentinsulzer
Copy link
Member

Have implemented this but it made simplify much slower :( will investigate ...

valentinsulzer added a commit that referenced this issue Nov 1, 2019
valentinsulzer added a commit that referenced this issue Nov 1, 2019
@TomTranter
Copy link
Contributor Author

Sympy allows pickling of symbols but not the lambda functions

import sympy as syp
import numpy as np
import pickle
import matplotlib.pyplot as plt

sto = syp.symbols('sto')
# graphite_mcmb2528_ocp_Dualfoil
u_eq = (0.194
        + 1.5 * syp.exp(-120.0 * sto)
        + 0.0351 * syp.tanh((sto - 0.286) / 0.083)
        - 0.0045 * syp.tanh((sto - 0.849) / 0.119)
        - 0.035 * syp.tanh((sto - 0.9233) / 0.05)
        - 0.0147 * syp.tanh((sto - 0.5) / 0.034)
        - 0.102 * syp.tanh((sto - 0.194) / 0.142)
        - 0.022 * syp.tanh((sto - 0.9) / 0.0164)
        - 0.011 * syp.tanh((sto - 0.124) / 0.0226)
        + 0.0155 * syp.tanh((sto - 0.105) / 0.029)
        )
u_eq_prime = u_eq.diff(sto)
u_eq_fn = syp.lambdify(sto, expr=u_eq, modules='numpy')
u_eq_prime_fn = syp.lambdify(sto, expr=u_eq_prime, modules='numpy')
S = np.linspace(0, 1.0, 101)
plt.figure()
plt.plot(S, u_eq_fn(S))
plt.plot(S, u_eq_prime_fn(S))
sympy_objs = [u_eq, u_eq_prime, u_eq_fn, u_eq_prime_fn]

with open("sympy_symbol", "wb") as f:
    pickle.dump(u_eq, f)

with open("sympy_objs", "wb") as f:
    pickle.dump(sympy_objs, f)

@TomTranter
Copy link
Contributor Author

Hi @tinosulzer thanks for your work on this so far. I already made a branch for this which I think @rtimms made a change in. Shall we just delete my one? issue-699-enable-pickle

@valentinsulzer
Copy link
Member

If the changes don't clash (it doesn't look like they do, but I couldn't tell what @rtimms changed) then we can just merge the two branches

@rtimms
Copy link
Contributor

rtimms commented Nov 1, 2019

i merged master into pybamm-pnm. was the new branch based off of that?

@valentinsulzer
Copy link
Member

The problem with setup becoming slower is not such a big deal if we move to using casadi as default (possible after #685 once interpolation works). The conversion to casadi is fast, only happens at the solver stage, and is only saved within the solver object, so we wouldn't necessarily have to pickle it.

How comfortable are we generally with moving to using casadi's automatic differentiation as default, with ours only as backup? Pros: very fast, saves reinventing the wheel; cons: sunk cost, ours would probably get out of date

@rtimms
Copy link
Contributor

rtimms commented Nov 6, 2019

I'm in favour of moving to casadi. It's far more sophisticated than anything we would want to implement ourselves. I think the time we spent time on our method isn't reason enough to stop us moving to something better. And for me at least, implementing stuff related to the jacobian helped me learn a lot about the codebase, debugging and python generally, so the time I invested personally isn't a sunk cost in my view.

@TomTranter
Copy link
Contributor Author

Did we not find out that casadi can't be pickled either. I think the solver would have to be pickled for multiprocessing. Is it worth me trying to get some basic multiprocessing set up, say for parametric study so we can test these options. In general I'm in favour of not re-inventing the wheel but I don't know much about casadi and how supported it will be, regularly it will be developed. Easy it is to install on various os. It would be good to have a little tutorial from @tinosulzer about it at our next dev meeting. Thanks for your hard work on it

@valentinsulzer
Copy link
Member

Definitely worth looking into multiprocessing :) it turns out you can pickle casadi once everything is set up, just not in the intermediate stages, but that is not a problem for us. https://web.casadi.org/blog/parfor/

valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
@martinjrobins
Copy link
Contributor

Note that there are alternative to the standard library multiprocessing library that explicitly avoid standard pickle, such as this one: https://joblib.readthedocs.io/en/latest/parallel.html. This uses cloudpickle, which can pickle lambda functions.

valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
valentinsulzer added a commit that referenced this issue Nov 12, 2019
@TomTranter
Copy link
Contributor Author

@martinjrobins thanks for the info. I will look into these alternatives as well but I read the link and there could be some significant slow downs potentially.

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

Successfully merging a pull request may close this issue.

4 participants