BUG Only set exceptions on ModelFuture if there was an error (#86)

In #79, the entire `if` gate in `ModelPipeline._set_model_exception` was removed, when I should only have removed the part after the `and`. If we don't have the `if an exception happened in the job` check, we'll set an exception on all `ModelFuture` objects, even if everything succeeded.

Author: Stephen Hoover

CHANGELOG.md

@@ -5,6 +5,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ## [Unreleased]
 
 ### Fixed
+- Fixed a bug which caused an exception to be set on all ``ModelFuture`` objects, regardless of job status (#86).
+- Fixed a bug which made the ``ModelPipeline`` unable to generate prediction jobs for models trained with v0.5 templates (#84).
 - Handle the case when inputs to ``ModelFuture`` are ``numpy.int64`` (or other non-``integer`` ints) (#85).
 
 ## 1.5.0 - 2017-05-11

civis/ml/_model.py

@@ -316,7 +316,7 @@ def _set_model_exception(fut):
         If it indicates an exception, replace the generic
         ``CivisJobFailure`` by a more informative ``ModelError``.
         """
-        # Prevent inifinite recursion: this function calls `set_exception`,
+        # Prevent infinite recursion: this function calls `set_exception`,
         # which triggers callbacks (i.e. re-calls this function).
         if fut._exception_handled:
             return
@@ -328,15 +328,16 @@ def _set_model_exception(fut):
             if fut.is_training and meta['run']['status'] == 'succeeded':
                 # if training job and job succeeded, check validation job
                 meta = fut.validation_metadata
-            try:
-                # This will fail if the user doesn't have joblib installed
-                est = fut.estimator
-            except Exception:  # NOQA
-                est = None
-            fut.set_exception(
-                  ModelError('Model run failed with stack trace:\n'
-                             '{}'.format(meta['run']['stack_trace']),
-                             est, meta))
+            if meta['run']['status'] == 'exception':
+                try:
+                    # This will fail if the user doesn't have joblib installed
+                    est = fut.estimator
+                except Exception:  # NOQA
+                    est = None
+                fut.set_exception(
+                      ModelError('Model run failed with stack trace:\n'
+                                 '{}'.format(meta['run']['stack_trace']),
+                                 est, meta))
         except (FileNotFoundError, CivisJobFailure) as exc:
             # If there's no metadata file
             # (we get FileNotFound or CivisJobFailure),

civis/ml/tests/test_model.py

@@ -287,6 +287,19 @@ def test_set_model_exception_unknown_error():
     assert str(err.value).startswith(err_msg)
 
 
+@mock.patch.object(_model.cio, "file_to_json", autospec=True,
+                   return_value={'run': {'status': 'succeeded',
+                                         'stack_trace': None}})
+def test_set_model_exception_no_exception(mock_f2j):
+    # If nothing went wrong, we shouldn't set an exception
+    ro = [{'name': 'model_info.json', 'object_id': 137, 'object_type': 'File'},
+          {'name': 'metrics.json', 'object_id': 139, 'object_type': 'File'}]
+    ro = [Response(o) for o in ro]
+    mock_client = setup_client_mock(1, 2, state='succeeded', run_outputs=ro)
+    fut = _model.ModelFuture(1, 2, client=mock_client)
+    assert fut.exception() is None
+
+
 class ModelFutureStub:
 
     def __init__(self, exc, trn, val):