Addressing PR comments

[juliaputko]

…ssing PR comments

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (preview@7bcb149). Click here to learn what that means.

❗ Current head 86746da differs from pull request most recent head 9de5069. Consider uploading reports for the commit 9de5069 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             preview     #528   +/-   ##
==========================================
  Coverage           ?   90.70%           
==========================================
  Files              ?       66           
  Lines              ?     4636           
  Branches           ?        0           
==========================================
  Hits               ?     4205           
  Misses             ?      431           
  Partials           ?        0           

Files	Coverage Δ
smartsim/_core/control/controller.py	86.05% <100.00%> (ø)
smartsim/_core/control/previewrenderer.py	96.07% <100.00%> (ø)
smartsim/entity/dbobject.py	67.88% <ø> (ø)
smartsim/entity/ensemble.py	99.26% <100.00%> (ø)
smartsim/error/errors.py	100.00% <100.00%> (ø)
smartsim/experiment.py	87.50% <100.00%> (ø)

[ankona]

Looks good.

I'd really like if we could avoid changing the templates if the directory structure changes. If we can't do that, all that's left are a couple nitpicks on docstrings.

[amandarichardsonn]

Super nice work and incredible amount of code, I can definitely tell a lot of hard work went into this! Here is what I came up with:

Documentation:
I have some suggestions below of where to include information on preview.

1. Make sure the new Experiment.preview is added to doc/api/smartsim_api.rst
2. Add preview to this chart here
3. Add preview as an Example step here

Preview templates:

1. I would add the path for Ensemble, Model and Orchestrator to the previews! It is possible that a user specifies their own path and therefore it is not located in the exp_path. The path arguments for Orch and Ensemble have been pushed to develop.
2. I notice that in an Experiment with multiple databases, the db_identifier for the standalone dbs are given, even if one is not provided. A colo db identifier is only shown if provided even though the identifier would be default: orchestrator -> should we show the colo db_identifier name always like for std?

Possible Bug 1:
I am running the two Model script below:

model = exp.create_model("hello_world", settings, path="./")
model_2 = exp.create_model("hello", settings, path="./")
model_2.colocate_db_tcp()
exp.preview(model, model_2)
exp.start(model, model_2)

The preview output is:

== Models ==

    = Model Name: hello_world =
        Colocated:
          Connection Type: TCP
          TCP/IP Port(s):
            6379
        Client Configuration:
            Database Backend: redis
            Connection Type: TCP
            TCP/IP Port(s):
              6379
            Type: Colocated


    = Model Name: hello =
        Colocated:
          Connection Type: TCP
          TCP/IP Port(s):
            6379
        Client Configuration:
            Database Backend: redis
            Connection Type: TCP
            TCP/IP Port(s):
              6379
            Type: Colocated

The preview suggests that both of the Models are colocated rather than the 1? Am I misunderstanding?

Document with more suggestions find here