[AIR] Support array output from Torch model #24902
Conversation
| # these cannot be used as values in a Pandas Dataframe. | ||
| # We have to convert the outermost dimension to a python list (but the values | ||
| # in the list can still be Numpy arrays). | ||
| return pd.DataFrame({"predictions": list(prediction)}, columns=["predictions"]) |
There was a problem hiding this comment.
Can we use TensorArray here now that #24752 is merged?
| return pd.DataFrame({"predictions": list(prediction)}, columns=["predictions"]) | |
| return pd.DataFrame({"predictions": TensorArray(prediction)}, columns=["predictions"]) |
There was a problem hiding this comment.
I didn't want to have a ray.data dependency in Predictors. Once we do the data type conversion cleanup and probably move TensorArray to a common utils file, I definitely agree though, we should use TensorArray here!
| unsqueeze (bool): If set to True, the features tensors will be unsqueezed | ||
| (reshaped to (N, 1)) before being concatenated into the final features | ||
| tensor. Otherwise, they will be left as is, that is (N, ). | ||
| Defaults to True. |
There was a problem hiding this comment.
Nit: out of scope for this PR, but this explanation doesn't really make sense for multi-dimensional inputs
There was a problem hiding this comment.
We could change the description, yeah. We are now calling unsqueeze which should work with an arbitrary number of dimensions and not just 2, like the view call we were using before.
There was a problem hiding this comment.
Agreed the semantics here need to be changed. Though this is out of scope for this PR- this PR is just updating the docstring to match the same as the argument name, which is a mistake we had before.
| def test_predict_array_output(): | ||
| """Tests if predictor works if model outputs an array instead of single value.""" | ||
|
|
||
| predictor = TorchPredictor(model=model, preprocessor=preprocessor) |
There was a problem hiding this comment.
Nit: this is definitely out of scope for this PR, but it might be better to define model and preprocessor in the test because:
- Less confusion when reading the tests since all of the code is visible from within each test.
- Less chance of sharing state between tests, which creates unwanted dependencies between them.
Related advice from .NET's unit testing best practices.
| def test_predict_array_output(): | ||
| """Tests if predictor works if model outputs an array instead of single value.""" | ||
|
|
||
| predictor = TorchPredictor(model=model, preprocessor=preprocessor) |
There was a problem hiding this comment.
Just for my own understanding, why do we need to use a preprocessor here?
There was a problem hiding this comment.
I believe it is to check that its applied, though that could be covered by other tests and not this one specifically
There was a problem hiding this comment.
In this case I'd vote to leave it out of this test. We should ideally only test one thing
There was a problem hiding this comment.
Sounds good, I updated the tests! PTAL!
There was a problem hiding this comment.
Generally looks good once @bveeramani's comments are addressed
| def test_predict_array_output(): | ||
| """Tests if predictor works if model outputs an array instead of single value.""" | ||
|
|
||
| predictor = TorchPredictor(model=model, preprocessor=preprocessor) |
There was a problem hiding this comment.
In this case I'd vote to leave it out of this test. We should ideally only test one thing
richardliaw
added
the
@author-action-required
amogkam
removed
the
@author-action-required
The previous implementation of
TorchPredictorfailed when the model outputted an array for each sample (for example outputting logits). This is because Pandas Dataframes cannot hold numpy arrays of more than 1 element. We have to convert the outermost numpy array returned by the model to a list so that it be can stored in a Pandas Dataframe.The newly added test fails under the old implementation.
Why are these changes needed?
Related issue number
Checks
scripts/format.shto lint the changes in this PR.