Skip to content

Conversation

@isabelizimm
Copy link
Contributor

@isabelizimm isabelizimm commented Feb 8, 2023

Added in this PR

  • removes ALL handling of data from vetiver_post
  • add helper functions for data handling to new file, helpers.py
  • some small refactoring of testing the API (eg, parameterizing values, removing duplicated tests in vetiver/tests/test_predict.py and vetiver/tests/test_sklearn.py)
  • updates error messages to be more descriptive of location and feature causing error

previous error message:

422 Unprocessable Entity 
TypeError: Predict expects DataFrame, Series, or dict.

new error message:

1 validation error for Request: body -> 0 -> disp:   value is not a valid float (type=type_error.float)

where 0 is the index of the error and disp is the feature with the error

Related Issues

Notes

This PR must be merged before #143 since spacy's input data of one column of strings cannot be handled in the same way as tabular data, and the output might be multiple columns. This PR gives the flexibility of input/output handling to each modeling framework's handler.

@isabelizimm isabelizimm marked this pull request as ready for review March 13, 2023 22:34
@isabelizimm isabelizimm requested a review from has2k1 March 13, 2023 22:34
@isabelizimm isabelizimm marked this pull request as draft March 13, 2023 22:44
@isabelizimm isabelizimm marked this pull request as ready for review March 13, 2023 22:58
@isabelizimm isabelizimm added this to the v0.2.1 milestone Mar 14, 2023
@isabelizimm isabelizimm requested a review from machow March 15, 2023 14:03
Copy link
Collaborator

@has2k1 has2k1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good to merge. The improvements can come afterwards. The important one is, full type-annotations that pass a type checker. It would make clearer how the input data, output data and api data are juggled.

@@ -1,4 +1,3 @@
from vetiver.handlers import base
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this (or immediately after this PR) is the good time to add a module docstring for all the whats and the whys about Handlers. I will do a PR after this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! That would be a great addition for more context on Handlers.

@isabelizimm isabelizimm merged commit a265da0 into main Mar 21, 2023
@isabelizimm isabelizimm deleted the server-refactor branch May 25, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

Consider importing rsconnect.api.RSConnectServer move all data preprocessing to handler Example dropdown improvement

3 participants