I want to come up with a simple convenience function to save and load pickle objects. A secondary objective is to simply execute the function without the above function. I have come up with two versions so far. Just want some opinions, which of the following will be better from a software engineering point of view. Or more pythonic. If possible please suggest a still better version, with changed parameter names or something already out there.
def test(filename, func, exec_only=False, ignore_file=False, **kwargs):
if exec_only:
obj = func(**kwargs)
else:
fname = Path(filename)
if fname.exists() and not ignore_file:
obj = jl.load(filename)
else:
obj = func(**kwargs)
jl.dump(obj, fname, compress=True)
return obj
Another version:
def test2(filename, func, exec_only=False, ignore_file=False, **kwargs):
if Path(filename).exists() and not ignore_file and not exec_only:
obj = jl.load(filename)
else:
obj = func(**kwargs)
if not exec_only:
jl.dump(obj, fname, compress=True)
return obj
Thanks a lot in advance.
EDIT: I created this function as a machine learning utility, since execution is typically very costly and the decision to execute is dependent on the presence of the file. I will be incorporating some features from Sam's answer. Thanks.
-
3\$\begingroup\$ Welcome to Code Review! I changed your title to state what your code is supposed to do, not your concerns about it. Your question could be even better if you'd show a little example on how you intend to use those functions, although their purpose is already reasonably clear. \$\endgroup\$AlexV– AlexV2020年02月06日 15:59:25 +00:00Commented Feb 6, 2020 at 15:59
1 Answer 1
A few general tips:
Have a function name that indicates what the function does. If it's hard to come up with a name that describes the function's purpose (e.g. because it does totally different things depending on what argument you pass it), it's a clue that the function needs to be broken up into multiple functions with clearer purposes. It looks like your function will either save OR load depending what arguments you pass it; this is pretty bad from a software design point of view because it makes it less obvious what it's doing from the caller's perspective, and harder to verify correctness from the implementation's perspective.
Use type annotations. These also help to make it clear what your function does, and they let you use
mypy
to automatically catch bugs. Note that**kwargs
are difficult to declare useful types for; any function call that takes**kwargs
is impossible to check statically. Given that yourkwargs
are only used to invokefunc
(I'm guessingfunc
is a constructor?), I would just put the responsibility on the caller to construct the object before passing it to you; that way if the function is typed, the checking will happen in the caller's frame, rather than having your function "hide" the types and permit bugs to happen.Don't have flags whose only purpose is to make the function a no-op. This is needlessly confusing; if the caller wants nothing to happen, they can just not call your function.
Given those suggestions I think you'd end up with two different (and very simple) functions:
from typing import Optional
def load_pickle(file_name: str) -> Optional[object]:
"""Load a pickled object from file_name IFF the file exists."""
return jl.load(file_name) if Path(file_name).exists() else None
def save_pickle(obj: object, file_name: str) -> None:
"""Pickle and save the object to file_name."""
jl.dump(obj, file_name, compress=True)
-
\$\begingroup\$ What is as
str -> None
... Alsoobject
overAny
seems like a bad suggestion to someone new to typehints. \$\endgroup\$2020年02月06日 17:20:55 +00:00Commented Feb 6, 2020 at 17:20 -
\$\begingroup\$ Maybe
file_name: str -> None
is supposed to befile_name: str) -> None
? \$\endgroup\$AlexV– AlexV2020年02月06日 17:24:15 +00:00Commented Feb 6, 2020 at 17:24 -
\$\begingroup\$ yup, typo.
object
is a little more specific thanAny
, and I think it's the type you'd want here (i.e. you can't pickleNone
, I don't think?). Ideally there'd be some more specificPickleable
type/protocol but I don't think any such thing exists. \$\endgroup\$Samwise– Samwise2020年02月06日 20:11:58 +00:00Commented Feb 6, 2020 at 20:11 -
\$\begingroup\$ It's more specific yes. But not how you're thinking, None falls under it as all classes are newstyle classes now - and so inherit from
object
. Also in Python 2 it may be incorrectly typing old-style classes. I'm sure we could play tennis with gotchas, which is why I don't think it's worth thinking about this in most usages of type hints. Especially for beginners. I agree with the idea ofPickleable
tho. \$\endgroup\$2020年02月06日 21:56:45 +00:00Commented Feb 6, 2020 at 21:56
Explore related questions
See similar questions with these tags.