Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update doc strings #290

Merged
merged 6 commits into from
May 30, 2023
Merged

Update doc strings #290

merged 6 commits into from
May 30, 2023

Conversation

mellis13
Copy link
Contributor

Per group discussion, updating doc strings to the correct form of colocated.

@mellis13 mellis13 requested a review from MattToast May 25, 2023 15:46
@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #290 (160560d) into develop (8a5e940) will decrease coverage by 0.07%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #290      +/-   ##
===========================================
- Coverage    87.65%   87.59%   -0.07%     
===========================================
  Files           58       58              
  Lines         3322     3322              
===========================================
- Hits          2912     2910       -2     
- Misses         410      412       +2     
Impacted Files Coverage Δ
smartsim/entity/model.py 97.14% <ø> (ø)
smartsim/experiment.py 80.43% <ø> (ø)

... and 1 file with indirect coverage changes

Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

Two quick questions regarding the jupyter changes, but otherwise lgtm!

"on the same compute hosts an a Model instance defined by the user. In this\n",
"deployment, the database is not connected together in a cluster and each shard\n",
"of the database is addressed individually by the processes running on that compute\n",
"host. This is particularly important for GPU-intensive workloads which require\n",
"frequent communication with the database.\n",
"\n",
"<img src=\"https://www.craylabs.org/docs/_images/co-located-orc-diagram.png\" alt=\"lattice\" width=\"600\"/>\n"
"<img src=\"https://www.craylabs.org/docs/_images/colocated-orc-diagram.png\" alt=\"lattice\" width=\"600\"/>\n"
Copy link
Member

@MattToast MattToast May 25, 2023

Choose a reason for hiding this comment

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

Just confirming that this link does correspond with the colocated-orc-diagram.png whose name was changed in this pr. Do we care that this may not render if a user were to pull this notebook from develop as opposed to from a release?

We have the docker container for tutorials that we expect users to use so I imagine very few are pulling and running the dev branch on bare metal, so I'm tempted to say its fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, after you raised it I'm reconsidering my original choice. develop should be fully functioning and the release is probably 4 weeks away. Let me change this back and then I'll have a separate ticket to update the file name right before launch.

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, I think I'm in favor of just not updating it at all. It's an internal filename and if we change it previous versioned releases will be broken because once we publish this to the main docs the file won't exist anymore. Do you agree with that? It's an edge case but I guess not much is gained by fixing an internal file name.

Copy link
Member

Choose a reason for hiding this comment

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

I think that seems like a perfectly reasonable justification to just leave the current filename! Like you said, its strictly internal, so I'm totally fine with letting this be one place where the "co-loctated" spelling still exists

@mellis13 mellis13 merged commit 99958a0 into CrayLabs:develop May 30, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants