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 inference and multinode guide page for DLRS stack #877

Merged
merged 6 commits into from
Jan 13, 2020

Conversation

bd-dean
Copy link
Contributor

@bd-dean bd-dean commented Oct 22, 2019

This will add the ai-inference instructions. Note that this document has some outstanding TODO issues, so it cannot be merged -- need to verify links & ENV values.

@bd-dean bd-dean requested a review from mvincerx October 22, 2019 23:51
@bd-dean bd-dean added the new New document or new UI feature/functionality for CL Docs. label Oct 22, 2019
Copy link
Contributor

@mvincerx mvincerx left a comment

Choose a reason for hiding this comment

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

Overall, looks great. The main challenge here is "How to better...":

  • Distinguish what is required vs. what's optional
  • Thread together all the subsections to explain how they relate to using DLRS with Seldon Core.
  • Define purpose. See sentence "we explore a solution for using the Deep Learning Reference Stack with a Seldon Core*..." A guide needs to propose a specific set of procedures to reach a goal, or optionally, multiple goals. Rather than explore, consider defining a specific path to completion.

source/guides/stacks/dlrs-inference.rst Show resolved Hide resolved
source/guides/stacks/dlrs-inference.rst Outdated Show resolved Hide resolved
source/guides/stacks/dlrs-inference.rst Outdated Show resolved Hide resolved
source/guides/stacks/dlrs-inference.rst Outdated Show resolved Hide resolved
********

The solution covered here uses the following software components:

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're missing an opportunity here. The Overview lacks a clear explanation of how all of these components are used in concert. A simple, succinct sentence will solve this like a shot to the heart. I recommend adding that "goal" statement here--relating how all these components fit together. Also, I'd like to see these components as short, uniform descriptions. I think what's missing here is the "keystone" sentence at the beginning (above); then each one of these descriptions can explain how it serves that goal.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bd-dean. BTW: For comparison, I like the intro sentences in the Overview of the main DLRS guide. Maybe you could write something similar here.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think?
IMHO: If this solution requires all of these components, it should use "requires" instead of "uses". I'm hoping there could be a simple, direct sentence without nuance.

@mvincerx mvincerx added the P2 Medium Priority label Oct 25, 2019
Copy link
Contributor

@mvincerx mvincerx left a comment

Choose a reason for hiding this comment

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

Thanks for all the commits and revisions. I have a few minor wording requests.
I was unable to complete validation on this use case because of proxy issues--after running helm init. See comments below.

source/guides/stacks/dlrs-inference.rst Show resolved Hide resolved
********

The solution covered here uses the following software components:

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think?
IMHO: If this solution requires all of these components, it should use "requires" instead of "uses". I'm hoping there could be a simple, direct sentence without nuance.

@mvincerx
Copy link
Contributor

@bd-dean. Thanks for the last few commits here. What are we waiting on for this PR? Any blockers?
This PR is at the "very good enough" stage.

@mvincerx
Copy link
Contributor

Hey @bd-dean. What's the status on this PR? Are there any blockers?

@bd-dean
Copy link
Contributor Author

bd-dean commented Dec 19, 2019

@mvincerx we’re waiting in clearance to release the feature and then can pull the trigger on this PR.

@mvincerx
Copy link
Contributor

Thanks for the update.

Copy link
Contributor

@mvincerx mvincerx left a comment

Choose a reason for hiding this comment

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

LGTM after minor edit is made.

@mvincerx
Copy link
Contributor

Hey @bd-dean. Are we still on hold for this?

@mvincerx
Copy link
Contributor

LGTM.

@mvincerx mvincerx merged commit ff5f2e4 into clearlinux:master Jan 13, 2020
@bd-dean bd-dean deleted the bd-dlrs-inference branch April 16, 2020 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new New document or new UI feature/functionality for CL Docs. P2 Medium Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants