-
Notifications
You must be signed in to change notification settings - Fork 37
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
Orchestrator docs #432
Orchestrator docs #432
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## smartsim-docs-refresh #432 +/- ##
========================================================
Coverage ? 90.47%
========================================================
Files ? 60
Lines ? 3738
Branches ? 0
========================================================
Hits ? 3382
Misses ? 356
Partials ? 0 |
doc/orch_hold_file.rst
Outdated
------- | ||
This example provides a demonstration on automating the deployment of | ||
a standard Orchestrator, connecting a SmartRedis Client from | ||
within the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incomplete sentence?
doc/orch_hold_file.rst
Outdated
^^^^^^^^^^^^^^^^^^^^^ | ||
To establish a connection with the standard database, | ||
we need to initialize a new SmartRedis client. | ||
Since the standard database we launch in the driver script |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link to the driver script?
doc/orch_hold_file.rst
Outdated
Next, create a NumPy tensor to send to the standard database to retrieve | ||
in the driver script by using ``Client.put_tensor(name, data)``: | ||
.. code-block:: python | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe inclue the imports needed for numpy a the top of the code block?
doc/orchestrator.rst
Outdated
can address the cluster with the SmartRedis clients like a single block of memory | ||
using simple put/get semantics in SmartRedis. The client establishes a | ||
connection using the database address and travels off the model compute node to | ||
the database compute node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a reader, I'm having a hard time understanding what ".. and travels off the model compute node to the database compute node." means?
Are we saying "the client establishes a connection and seamlessly handles communication with nodes in the cluster"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was attempting to say that the client is initialized on the model node, and then travels off the node to the database nodes further emphasizing that the model and orch do not share compute resources but this might be more of a developer explanation and not something the user needs to worry about
doc/orchestrator.rst
Outdated
`db_identifier` argument that required when initializing an Orchestrator or | ||
colocated Model during a multi database Experiment. When initializing a SmartRedis | ||
client during the Experiment, first create a ``ConfigOptions`` object | ||
with the `db_identifier` argument created during before passing object to the Client() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonky: "argument created during before passing object to the..."
Consider editing this whole paragraph again. it seems like it got lost at the end of the file!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the docs updates -- initial pass through with some suggested changes and questions.
doc/orchestrator.rst
Outdated
capable of storing numerical data (Tensors and Datasets), AI Models, and scripts (TorchScripts). | ||
Combined with the SmartRedis clients, the ``Orchestrator`` is capable of hosting and executing | ||
AI models written in Python on CPU or GPU. The ``Orchestrator`` supports AI Models written with | ||
TensorFlow, Pytorch, TensorFlow-Lite, or models saved in an ONNX format (e.g. sci-kit learn). | ||
|
||
.. |orchestrator| image:: images/Orchestrator.png |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just putting a note here because we might have a better image we want to leverage. However, if we do replace this image and it is used elsewhere, we have to keep it in the repo or v0.6.0 docs will be broken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced the image with something I found in doc/images - but I have not glanced through orch images in presentations yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks promising! I think we should be consistent with how we define the different database deployments. I like colocated and standalone, leaving sharded only to describe the special multi-node case of standalone, but I have no strong opinion. Whatever descriptions we adopt, we should make sure we use them across all the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple more questions and comments -- thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of small comments. Most of the comments are regarding when we should tag a smartsim term like orchestrator
. Let's raise that to the group to get consensus. Otherwise, I think this PR is nearly ready to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor comments but then I think we are good to merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
9baf6cd
into
CrayLabs:smartsim-docs-refresh
This feature branch holds all documentation for the Orchestrator and will be merged to the main SmartSim doc branch.