ContainerFuture propagates error from logs #416
Conversation
civis/tests/test_parallel.py
Outdated
| @@ -376,8 +376,9 @@ def test_result_success(mock_civis): | |||
| # Test that we can get a result back from a succeeded job. | |||
| callback = mock.MagicMock() | |||
| mock_civis.io.civis_to_file.side_effect = make_to_file_mock('spam') | |||
| fut = ContainerFuture(1, 2, client=mock.MagicMock()) | |||
| fut.set_result(Response({'state': 'success'})) | |||
| mock_client = create_client_container_mock(1, 2, state='success', | |||
There was a problem hiding this comment.
ContainerFuture now has a callback during init, so we can't get away with initializing and setting a result directly during testing. I got around this by moving the container-focused client mock from CivisML to the general tests and using it when initializing the future.
| res = civis.parallel._CivisBackendResult(fut, callback) | ||
| fut._set_api_exception(CivisJobFailure( |
There was a problem hiding this comment.
h/t @stephen-hoover for noticing that the arguments to _set_api_exception were not actually the expected order/type here (although the test happened to pass anyway).
| @@ -527,7 +502,7 @@ def _set_model_exception(fut): | |||
| # If there's no metadata file | |||
There was a problem hiding this comment.
GitHub won't let me comment too far away from this PR's changes; so this is the closest I can get. This comment is for the new L474, def _set_model_exception(fut). I suggest that you rename ModelFuture._set_model_exception to ModelFuture._set_job_exception. That way the call to the superclass's __init__ will set the subclass's version of the callback. With the current code, it looks like we'll never reach the _set_model_exception callback, since we'll first call ContainerFuture._set_job_exception, and that will set _exception_handled to True.
There was a problem hiding this comment.
Hi Stephen, I updated the name and took out the repeated callback attributes in the init. It seems to be working properly, but I had to move some of the other attributes being set in the init to above the super().__init__() since the _set_job_exception method relies on some of those properties being set.
civis/tests/mocks.py
Outdated
| c = mock.Mock() | ||
| c.__class__ = APIClient |
There was a problem hiding this comment.
How about starting with the auto-specced client mock? The autospecs can be very helpful with testing. There's also a couple of places where the return of this function is now replacing create_client_mock(), so those tests would lose their autospecs.
| c = mock.Mock() | |
| c.__class__ = APIClient | |
| c = create_client_mock() |
|
@stephen-hoover @mheilman Sorry about the long wait period. I tried to address all of the remaining feedback. |
|
@ggarcia-civis , I'm unlikely to be able to re-review in a timely fashion, so I will leave this review to @mheilman . |
stephen-hoover
dismissed
their stale review
Unable to re-review to validate changes.
civis/ml/tests/test_model.py
Outdated
| @@ -379,13 +379,16 @@ def test_set_model_exception_unknown_error(): | |||
| fut = _model.ModelFuture(1, 2, client=mock_client) | |||
| with pytest.raises(CivisJobFailure) as err: | |||
| fut.result() | |||
| print(str(err)) | |||
There was a problem hiding this comment.
Is this left over from debugging?
|
Hi @elliothevel, Mike is on PTO this week. Can you pick up on this PR review? |
…re (#426) * ENH add job and run IDs to CivisJobFailure * MAINT update changelog * ENH move #416 changes from ContainerFuture to CivisFuture * FIX adjust ContainerFuture and tests * ENH log retrieval works for CivisFuture more generally * RM _poll_and_set_api_result (should've been rm-ed at #425) * MAINT update changelog * TST make ModelFuture tests work with client.jobs.list_runs_logs * TST update mock client for ModelFuture tests * TST more clean-up for ModelFuture tests * FIX comment * ENH rewrite logic to add job/run ID in CivisJobFailure * TST refactor _create_poller_mock * TST poller call count for poll_on_creation True/False * ENH more natural log message order * ENH wait for delayed traceback in log * ENH clearer error message * ENH simplify error message a bit * TST revert unnecessary tweak
The Civis API currently returns
'error': Nonewhen a Python/R/container script fails, so API client users don't get any information about what caused the failure. CivisML works around this by returning error logs to the user in these cases. This PR moves this behavior fromModelPipelinetoContainerFutureso that users can get more informative errors when running jobs outside of CivisML as well.This is something of a workaround, especially since R client users won't get the benefits. Longer-term, we'd like to get more informative errors directly from the API.