3
\$\begingroup\$

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.

\$\endgroup\$
1
7
\$\begingroup\$

A few general tips:

  1. 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.

  2. 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 your kwargs are only used to invoke func (I'm guessing func 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.

  3. 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)
\$\endgroup\$
4
  • \$\begingroup\$ What is as str -> None... Also object over Any seems like a bad suggestion to someone new to typehints. \$\endgroup\$
    – Peilonrayz
    Feb 6 '20 at 17:20
  • \$\begingroup\$ Maybe file_name: str -> None is supposed to be file_name: str) -> None? \$\endgroup\$
    – AlexV
    Feb 6 '20 at 17:24
  • \$\begingroup\$ yup, typo. object is a little more specific than Any, and I think it's the type you'd want here (i.e. you can't pickle None, I don't think?). Ideally there'd be some more specific Pickleable type/protocol but I don't think any such thing exists. \$\endgroup\$
    – Samwise
    Feb 6 '20 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 of Pickleable tho. \$\endgroup\$
    – Peilonrayz
    Feb 6 '20 at 21:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.