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

Doc tweaks for paper resubmission #374

Merged
merged 34 commits into from
Aug 25, 2020
Merged

Conversation

dylanbannon
Copy link
Contributor

@dylanbannon dylanbannon commented Aug 12, 2020

  • Updating Kiosk diagram, both XML file for draw.io and resulting PNG.

  • Adding section to developer docs about verifying benchmarking numbers.

@dylanbannon dylanbannon self-assigned this Aug 12, 2020
@dylanbannon dylanbannon added the documentation A change in documentation is requested label Aug 12, 2020
@dylanbannon dylanbannon changed the base branch from master to stable August 12, 2020 00:11
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/images/Kiosk_Architecture.xml Outdated Show resolved Hide resolved
@willgraf
Copy link
Contributor

I've built it via RTD. There are definitely rendering problems. It looks like you're trying to write Markdown, but this is RST.

docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
@MekWarrior
Copy link
Collaborator

The docs will built it via RTD highlight a number of remaining rendering issues. The use of bold in certain contexts is odd and bullet style is inconsistent. Neither match the rest of the docs. I would also prefer the figure making portion be moved to the figure repo. Even with that, this section constitutes over half of the developer documentation. If it is going to remain this verbose and fastidious, perhaps it would be better to house it on a different page?

@dylanbannon
Copy link
Contributor Author

dylanbannon commented Aug 14, 2020

@MekWarrior
I'll remove the bold and reorganize things to that it's hopefully more concise. After that, I'm absolutely open to moving the benchmarking documentation to its own file if it still makes sense.

Moving the figure creation to the publication-figures repo doesn't quite make sense to me, since the figure recreation section here seems to dovetail well with the instructions in the README of the other repo. Is there some other way to make this section fit in better with the rest of the doc?

@MekWarrior
Copy link
Collaborator

@dylanbannon my supposition would be that we have a number of different repos that we point to from this one. As the figure making is already self contained, it would seem consistent with our approach that one would come to this portion of the docs for information on benchmarking and to the figure repo for information on figure making (and there's no reason we can't cross-link the 2). Moreover, it would help unclutter the docs on our primary repo, making it easier for users to parse and use.

@willgraf
Copy link
Contributor

It does seem natural to point them to the publication repo and say something like:

With the completed benchmarking runs you can use our graph generation repository to regenerate your own charts.

@dylanbannon
Copy link
Contributor Author

Ok, I pushed a rearranged commit. So, you guys think everything in the two figure recreation subheadings should be moved to the publication-figures repo and replaced with a pointer?

@dylanbannon
Copy link
Contributor Author

dylanbannon commented Aug 14, 2020

Also, I merged the version2 branch into master in the publication-figures repo, like you suggested @MekWarrior. I'm still having an issue with cloning the repo, though. Can anyone recreate vanvalenlab/publication-figures#2?

docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@willgraf willgraf left a comment

Choose a reason for hiding this comment

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

The storage bucket icon is much improved!

I don't like that arrows between Redis/Frontend and Redis/Autoscaler are off center. For some reason it really stands out to me.

Maybe we can move the purple GPU zone further into the corner to allow the straight line between redis and the frontend to be in the middle of the frontend icon?

I think the TF/ConsumerB arrow needs to be closer to the center of the TF icon as well, it looks weird to me that it's so close to the edge.

docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@willgraf willgraf left a comment

Choose a reason for hiding this comment

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

I'm pretty satisfied with both the text and the image, but we should get input from both @MekWarrior and @vanvalen.

@MekWarrior
Copy link
Collaborator

I think the architecture diagram looks much better. I feel like there are still some issues with spacing and centering, but I could be ok with this if vanvalen approves. Text reads well.

@willgraf
Copy link
Contributor

willgraf commented Aug 21, 2020

I was looking at the image a bit more, and the blank space inside the purple GPU zone is bothering me. I'll see if I can mock up some changes that might help.


I took a stab at fixing some things, and just opened a PR for the changes. It's not necessary to merge it in, but I thought it would help better show some of the things I wanted fixed.

@willgraf willgraf self-requested a review August 24, 2020 22:16
willgraf
willgraf previously approved these changes Aug 24, 2020
Copy link
Contributor

@willgraf willgraf left a comment

Choose a reason for hiding this comment

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

I made a last-minute change to the image (Consumer A was on both icons). It all looks good to me now.

@dylanbannon Thanks for your patience in dealing with so much of my nitpicky feedback!

Copy link
Collaborator

@MekWarrior MekWarrior left a comment

Choose a reason for hiding this comment

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

2 very minor changes and I'm good to go.

docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
docs/source/DEVELOPER.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@MekWarrior MekWarrior left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for the changes

@willgraf willgraf self-requested a review August 25, 2020 02:09
Copy link
Contributor

@willgraf willgraf left a comment

Choose a reason for hiding this comment

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

💯 🥇

@vanvalen vanvalen merged commit 384fb9d into stable Aug 25, 2020
@dylanbannon dylanbannon deleted the doc_tweaks_for_paper_resubmission branch August 25, 2020 18:12
willgraf added a commit that referenced this pull request Aug 27, 2020
* Fix documentation badge (#354)

* Bump redis-consumer versions to 0.6.1. (#355)

* If no default-region is selected, use us-west1 (#358)

* Clarify Quota increase documentation (#359)

* GPUS_ALL_REGIONS does not always exist in the quota, if not just set it to minimum. (#362)

* Fix last references to old project name. (#364)

* set prometheusOperator.cleanupCustomResource to true to delete CRDs when the chart is deleted. (#365)

* Warn the user that multizone clusters will have additional egress fees. (#366)

* Remove deprecated helmfiles. (#368)

* General documentation fixes. (#360)

* update documentation in version to 1.2.1 in preparation for pending release. (#371)

* Doc tweaks for paper resubmission (#374)

* Set the KUBERNETES_VERSION to 1.15. 1.14 is no longer supported. (#376)

Co-authored-by: willgraf <[email protected]>
Co-authored-by: dylanbannon <[email protected]>
Co-authored-by: MekWarrior <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation A change in documentation is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants