Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Inputs Key from Truss Templates #244

Merged
merged 7 commits into from
Mar 9, 2023

Conversation

philipkiely-baseten
Copy link
Member

@philipkiely-baseten philipkiely-baseten commented Mar 6, 2023

Hey! This is a big PR with lots of changes. Hoping to get this carefully reviewed and merged to close https://linear.app/baseten/issue/BT-5934/update-truss-model-templates-to-new-input-spec

Background

When we first created Truss, we required the predict function’s input to be a dictionary with the key “input.” This was necessary at the time, but also annoying, so when it was no longer necessary, we removed the requirement.

Problem

Despite removing the required input structure, the input key is still used by default in new Trusses. And is all over the docs and examples and product.

Solution

Revise the Truss templates for each supported framework to no longer use the inputs key.

Migration & Release

I would like this change to trigger a minor version release, updating Truss to 0.4.0. While it is not strictly a breaking change, nor is it super material for existing users (who likely have stopped using inputs on custom Trusses) it still feels like something that would be wrong to do just a patch for.

Testing

I have:

  • Updated unit tests, all pass
  • Manually tested all ipython notebooks
  • Manually tested new example pre- and post-processing functions
  • Manually tested multiple examples

Other PRs

Associated changes in other repos (all need reviews too):

Model Zoo

I am choosing NOT to change model zoo models at this time as this could break stuff and they have already been operating in the "inputs key is optional" world for a while now.

@linear
Copy link

linear bot commented Mar 7, 2023

BT-5934 Update Truss model templates to new input spec

We want to:

  • Remove the "inputs" keyed dictionary from being the default.
  • Change the "request" variable name to something more meaningful
  • Update the Truss docs by rebuilding models after this change (philip will do once change is live)

Context: https://basetenlabs.slack.com/archives/C01RWP3BA6N/p1667062781296179

Copy link
Collaborator

@pankajroark pankajroark left a comment

Choose a reason for hiding this comment

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

The types for all the postprocess functions need to be updated. The type of input to postprocess should match the type of output of predict, and the out of postprocess should be Any. lgtm otherwise.


def postprocess(self, request: Dict) -> Dict:
def postprocess(self, model_output: Dict) -> Dict:
Copy link
Collaborator

Choose a reason for hiding this comment

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

These types should be changed to any as well, they should also be Any.

Copy link
Member Author

Choose a reason for hiding this comment

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

The output type of predict is always a dictionary unless the user changes that behavior (which would mean they are in the model.py file and could change the type). And then that dictionary gets passed through and postprocess returns a dictionary too. Do you mean that it should be

def postprocess(self, model_output: Dict[str: List]) -> Any:?

Copy link
Collaborator

Choose a reason for hiding this comment

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

technically, we want this to be any type that can be serialized (same as for input). it's not very simple to put that in a typing statement, so any suffices.

I think the template determines what is idiomatic for users. If we put Dict then they'll start using Dict. If we put Any, they'll do whatever they want. I personally think we need to become less opinionated about things that don't matter and this might be one of them, so Any is preferred.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case, if they are all Any, should we just remove type hints altogether from predict?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can, but this will cause lint errors. we need type hinting so that linting doesn't fail. It's also a useful hint to have Any in my opinion.

@philipkiely-baseten philipkiely-baseten merged commit c5b89a6 into main Mar 9, 2023
@philipkiely-baseten philipkiely-baseten deleted the philip/rm-inputs-key branch March 9, 2023 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants