Customize Model via subclassing, clean up public-facing Model+Viz api

[jwayne]

• Changes to Model + subclasses

  • Make it so that to use a model, you subclass the relevant interface and
    override its attributes (e.g., preprocess, predict), rather than pass
    these attributes in as arguments to the constructor.
  • Call load in the constructor, since (1) none of instance's attributes
    have defined values before load is called which is weird, (2) we only
    expect to call load once, (3) we always call load right after
    initializing the instance anyway.
  • Make load private (--> _load) since it's only called in the constructor.
  • Update generate_model --> get_model and how config files are loaded
    accordingly.
  • Rename preprocess(targets) --> preprocess(raw_inputs) [why did we
    call it targets in the first place?]
  • Get rid of predict which isn't used, and rename _predict to predict
    (which still isn't used, but at least now we don't have an unused wrapper
    around the unused method).
  • Get rid of postprocess, since it is specific to SaliencyMap, and we can
    also achieve the same without defining that function by inspecting the
    shape of the model's input image tensor.
  • In examples, don't override decode_prob if the implementation matches the
    default. Accordingly, I got rid of the warnings in the interface.
  • Rename Model to BaseModel to match BaseVisualization
  • Define public instance attributes as properties
  • Rename class attributes to be all-caps
  • Move lines outside of try/except block if they're not meant to be caught

• Changes to BaseVisualization + subclasses

  • Define public instance attributes as properties
  • Rename class attributes to be all-caps
  • To see if an attribute was overridden in the subclass, define the attribute
    to be None in the interface, then check for None rather than checking
    if the attribute exists

• Formatting (I only fixed this when I saw it)

  • Remove end-of-line backslashes
  • In generator expressions, when 'for x in y' is split into multiple lines,
    put the 'for' on a new line
  • Add periods to comments, esp in docstrings (comments look more complete
    this way. Not a big deal, but a good habit leftover from my DE Shaw
    internship boss, who is a Python guru)
  • Instead of writing dict.update({'key': value}) when updating with a single
    key, write dict['key'] = value

[jwayne]

Proposals (these are probably more controversial):

• Model
  • Rename modules: ml_frameworks --> models, ml_frameworks.model -->
    models.base, flatten submodule structure within the new models module
    (it should contain base, tensorflow, keras).
  • Expose classes defined in submodules of the new models module at the top
    level.
  • Rename 'config.py' in examples to 'settings.py', to match the term
    PICASSO_SETTINGS and the default settings.py?
  • Rename decode_prob --> annotate_predictions?
• Visualization
  • Not a fan of reference_link - seems rather non-general. Would rather
    just include the link to the paper in the docstring.
• I think we can delete picasso_viz.egg_info?
• Formatting
  • What's reading these (:obj:tf.Tensor) strings?
  • 2 spaces to indent Args, Returns?
• I'd like the "Picasso visualizer" title to link to "/", not Github. Rather
  non-intuitive that it links to Github right now imo.

[jwayne]

Future (I can add these as issues)

• Model
  • Support multiple inputs (have to handle template too) (specifying multiple feeds in BACKEND_TF_INPUT_VAR #12)
  • Support SavedModel loading (Support tensorflow SavedModel loading #14)
• Visualization
  • Determine visualization to load via url, not cookie (Better URLs #15)
  • Specify available visualizations via a config parameter (Specify available visualizations via a config parameter #16)
  • Generalize html template naming [can you elaborate? -ryan]
  • Move BaseVisualization --> visualizations.base (bad practice to define
    stuff in init), then expose classes in module at top level [agree]
• Overall
  • Do we want to let users develop within the package (current situation)
    or outside of it (like Django)? This point is a bit vague, I can elaborate in person or later.

[codecov-io]

Codecov Report

Merging #10 into master will decrease coverage by 5.24%.
The diff coverage is 80.48%.

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
- Coverage   89.27%   84.03%   -5.25%     
==========================================
  Files          11       11              
  Lines         457      426      -31     
==========================================
- Hits          408      358      -50     
- Misses         49       68      +19

[jwayne]

@rhsimplex I think the code coverage will be alright once you merge #9 in, and I rebase on top.

[rhsimplex]

We'll split this up into smaller parts when you get back -- it's too difficult to merge with the trickle of bug fixes coming in which require continuous rebasing. Also I don't want the load method in the constructor since it is potentially a very costly operation and should be explicitly invoked.

I suggest we do it this way, which should minimize work for you and make it very easy to merge:

• One PR for all formatting changes and variable name changes

  • Rename preprocess(targets) --> preprocess(raw_inputs) [why did we call it targets in the first place?]
  • Rename Model to BaseModel to match BaseVisualization
  • Rename class attributes to be all-caps
  • Remove end-of-line backslashes
  • In generator expressions, when 'for x in y' is split into multiple lines,
    put the 'for' on a new line
  • Add periods to comments, esp in docstrings (comments look more complete this way. Not a big deal, but a good habit leftover from my DE Shaw internship boss, who is a Python guru)
  • Instead of writing dict.update({'key': value}) when updating with a single key, write dict['key'] = value
  • Get rid of predict which isn't used, and rename _predict to predict (which still isn't used, but at least now we don't have an unused wrapper around the unused method).

• One PR for non-breaking refactors

  • In examples, don't override decode_prob if the implementation matches the default. Accordingly, I got rid of the warnings in the interface.
  • Move lines outside of try/except block if they're not meant to be caught
  • Define public instance attributes as properties
  • To see if an attribute was overridden in the subclass, define the attribute to be None in the interface, then check for None rather than checking if the attribute exists

• One PR for the breaking change. We can update the documentation in this PR as well.

  • Update generate_model --> get_model and how config files are loaded accordingly.
  • Make it so that to use a model, you subclass the relevant interface and override its attributes (e.g., preprocess, predict), rather than pass these attributes in as arguments to the constructor.
  • Get rid of postprocess, since it is specific to SaliencyMap, and we can also achieve the same without defining that function by inspecting the shape of the model's input image tensor.

I hope it's clear why I'm requesting this. If not, let me know.

[jwayne]

OK @rhsimplex I will go ahead and refactor the PR as you asked. I like the way you broke up the changes. I see the benefits of the clearer history/review. In my head, I still have a bias against breaking up this PR since I tend to put higher weight on reducing code change overhead than on keeping clear git history/reviews. Clear git history is nice, but I don't like when it discourages micro improvements to code. I will keep track of how long it ends up taking me to do so and re-evaluate my bias - I always have room to learn here.

As for your suggestion on having load be called separately, what did you think about my proposed reasons?

Call load in the constructor, since (1) none of instance's attributes have
defined values before load is called which is weird, (2) we only expect to
call load once, (3) we always call load right after initializing the instance
anyway.

I agree with you in that I don't like the constructor being so costly, but I also don't like letting users initialize an object whose attributes and methods don't mean anything until load is called (i.e., (1)). Do you know of any examples of where other packages handle similar situations?

[jwayne]

Also @rhsimplex can you comment on my proposals (2nd comment in this PR)?

[jwayne]

I just spent 1hr 30min trying to separate out the PR :(

Let's talk in meatspace tomorrow!