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

Add user docs for Turing/Turing SDK #174

Merged
merged 13 commits into from
Mar 17, 2022

Conversation

deadlycoconuts
Copy link
Contributor

@deadlycoconuts deadlycoconuts commented Mar 7, 2022

In PR #164, PR #165, PR #170 and PR #171, an automated pyfunc ensembler building feature was introduced for Turing. This PR introduces a small section to guide users on how this feature can be used when deploying routers via the UI.

In addition, this PR also adds new documentation for router deployment using Turing SDK, which was introduced in PR #148, PR #152, PR #155.

Additions

  • docs/* - Addition of pyfunc ensembler deployment using Turing UI
  • sdk/docs/* - Addition of new documentation for router deployment using Turing SDK

Fixes

  • Tiny code cleanup to Turing SDK

@deadlycoconuts deadlycoconuts requested a review from a team March 7, 2022 08:13
@deadlycoconuts deadlycoconuts force-pushed the add_user_docs branch 2 times, most recently from ba4b1aa to 61f502a Compare March 11, 2022 04:28
@deadlycoconuts deadlycoconuts marked this pull request as ready for review March 11, 2022 08:26
@deadlycoconuts deadlycoconuts self-assigned this Mar 15, 2022
@deadlycoconuts deadlycoconuts deleted the add_user_docs branch March 15, 2022 03:18
@deadlycoconuts deadlycoconuts restored the add_user_docs branch March 15, 2022 03:18
Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

Hi @deadlycoconuts, left some comments on the contents of the docs. They look great - thanks for the extensive documentation!

I have one question - how will these multi-file .md docs be published to Pypi? I think that typically, only one file may be included as a README (ref), in one of the supported formats, of which .md is one. If we have multiple .md files, they need to be combined somehow. Have you already explored this?

Also, setup.py needs to be updated to include the doc when packaging and publishing to Pypi.

docs/how-to/create-a-router/configure-ensembler.md Outdated Show resolved Hide resolved
docs/how-to/create-a-router/configure-ensembler.md Outdated Show resolved Hide resolved
sdk/docs/README.md Outdated Show resolved Hide resolved
sdk/docs/README.md Outdated Show resolved Hide resolved
sdk/docs/README.md Outdated Show resolved Hide resolved
Each time a Turing Router sends a request to a
[pyfunc ensembler service](../../../../docs/how-to/create-a-router/configure-ensembler.md#pyfunc-ensembler),
`ensemble` will be called, with the request payload being passed as a `dict` object for the `features` argument, and
the route responses as a `list` of `dict` for the `predictions` argument.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The key of the dict is the route name, correct? (would help to state that explicitly)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the route responses will get returned as a List. Each element corresponds to a route response, which itself is a Dict which has the schema:

{
  "route": // ... Route name,
  "data": {
    // ... Successful route response
  },
  "is_default": // ... Boolean value
}

I suppose you were wondering if the underlying implementation of the pyfunc ensembler engine helps transform the raw route responses into something more intuitive, i.e. a Dict with key-value pairs where the key's a route name and the value's the route response. In this case the answer's no because I thought preserving the original request schema would be most easily understood by users (this would be how a user would expect a request if he/she was building a Docker ensembler), but that said, we can also implement the transformation in the engine if you think it's more intuitive for users.

sdk/docs/how-to/build-routers/02-router-version.md Outdated Show resolved Hide resolved
sdk/docs/how-to/build-routers/05-experiment-config.md Outdated Show resolved Hide resolved
sdk/docs/how-to/build-routers/06-resource-request.md Outdated Show resolved Hide resolved
sdk/docs/how-to/manage-routers-with-turing-api/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

Based on the recent discussions, my suggestion would be to:

  • Keep the sdk/docs/README.md as is (the image URL may need to be replaced with a public one, from Github later on, so Pypi can read from it)
  • Update the SDK samples, if required, with more comments (like what you've added for some of the how-to docs) / examples on the SDK usage (especially the Pyfunc real-time ensembler)
  • Drop the other how-to docs and handle the Sphinx to RTD in a separate task / PR later on.

sdk/setup.py Outdated Show resolved Hide resolved
]
}
)
print("Ensembler created:\n", ensembler)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new file to demonstrate the deployment of a router with a pyfunc ensembler.

Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

Left a couple of minor comments. Looks great otherwise. Thanks, @deadlycoconuts !

sdk/docs/README.md Outdated Show resolved Hide resolved
sdk/setup.py Outdated Show resolved Hide resolved
@deadlycoconuts
Copy link
Contributor Author

Thanks a lot for the thoughtful comments @krithika369 ! I'll be merging this! 🚀

@deadlycoconuts deadlycoconuts merged commit 335efc9 into caraml-dev:main Mar 17, 2022
@deadlycoconuts deadlycoconuts deleted the add_user_docs branch March 17, 2022 06:49
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.

2 participants