-
Notifications
You must be signed in to change notification settings - Fork 36
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
SmartSim Experiment docs #422
SmartSim Experiment docs #422
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## smartsim-docs-refresh #422 +/- ##
=========================================================
+ Coverage 90.34% 90.47% +0.13%
=========================================================
Files 60 60
Lines 3748 3738 -10
=========================================================
- Hits 3386 3382 -4
+ Misses 362 356 -6 |
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 is looking really good. A few places that need changes and a big TBD that has to be addressed before merging
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.
Great start to the documentation and an impressive amount of work. Most of my suggestions are fixing some inaccuracies of the descriptions which should hopefully be easy to fix. LEt me know if there's anything you want to clarify.
doc/experiment.rst
Outdated
steering, and more. The ``Orchestrator`` can be thought of as a general | ||
feature store capable of storing numerical data, ML models, and scripts. | ||
The orchestrator is capable of performing inference and script evaluation on feature | ||
store data. Any SmartSim ``Model`` or ``Ensemble`` model can connect to 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.
'Any SmarSim model or Ensemble of models' ? Technically an ensemble is a group of models.
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 refer to a single ensemble model here since we can instruct a single ensemble model to connect to an orch, and then another does not need to!
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 doing this refactor to the docs! Just some comments to hopefully make things clearer.
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 really like this refactor. Thanks for the hard work. Just a couple of small comments and rework before merging.
|
||
.. code-block:: bash | ||
If the systems launcher cannot be found or no `launcher` argument is provided, the default value of | ||
`launcher="local"` will be used. |
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.
@al-rigazzi Do we have.a method for viewing the launcher that is chosen by the auto
feature? I only see the private member attribute Experiment._launcher
.
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.
Very small comments and changes -- otherwise looks really good!
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.
Very tiny changes but then LGTM!
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!
This PR merges the Experiment.rst file into a branch that will encapsulate the entirety of the SmartSim documentation refactor. The Experiment.rst file offers documentation for an Experiment, a Launcher, the Experiment entities and an example demonstrating initializing, starting and stopping from within an experiment. The documentation is aimed to be accessible.