-
Notifications
You must be signed in to change notification settings - Fork 394
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
Added Studio docs #2455
Added Studio docs #2455
Conversation
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.
High-level comments before full review:
- in the long/medium term as per READMEs vs Docs #2443 needs to be moved to
viewer.iterative.ai/doc
or similar?- images perhaps to be hosted under a
studio
folder in https://github.com/iterative/static/ - would make it easier to host docs somewhere else without binary bloat- studio logo: related to add logos static#1
- images perhaps to be hosted under a
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.
Structure and titles review (will commit a few of these):
content/docs/studio/visualize-exp.md
Outdated
@@ -0,0 +1,42 @@ | |||
# Visualize and compare experiements |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Should "compare" be reflected in the sidebar label?
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.
Ideally yes. But that will make the sidebar item too long. Any suggestions on how to keep it short?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
we don't have time for this... Fix low hanging fruits
Yeah my initial review only focused on the overall structure with that same idea. And before reading all the content I was pretty confused by the titles. Isn't a "View" a visualization?
Reading into the View page, it's defined as "an interactive representation of the experiments run in your ML project," which sounds the same as visualizing experiments.
After reading the content in this page in detail I see that we mean specific experiments, which makes total sense. But before that this was confusing.
But yes I see now that it's not low-hanging fruit, so let's just leave this unresolved for now and we'll revisit later. 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.
Added to new ticket #2468
@tapadipti please |
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.
Good stuff.
Half the full review so far 🚧
Feel free to contest suggestions.
Co-authored-by: Jorge Orpinel <[email protected]> Co-authored-by: Casper da Costa-Luis <[email protected]>
@@ -0,0 +1,457 @@ | |||
<?xml version="1.0" encoding="UTF-8" standalone="no"?> |
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.
@jorgeorpinel @casperdcl Do you know who creates the logo/icon SVGs? I created a grayscale one for Studio from the color one using a tool called Inkscape, but I think a simpler one could be created. Not sure who would be doing this.
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've been concerned about resource fragmentation (full summary in iterative/static#1) - particularly for logos and the issues it's been causing with updating (e.g. iterative/cml.dev#50). In fact I've already been working on optimised monochrome versions (iterative/static#1 (comment)) using Inkscape amongst other tools. I guess @arcticbear can advise?
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.
@tapadipti @casperdcl I can help with that, what sizes and formats do you need?
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.
A small svg to be used in the sidebar (in place of the bullet point). The one I am currently using can be found here https://dvc-org-studio-docs-vntpxfoqdz.herokuapp.com/doc
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.
Added to new ticket #2469
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.
@tapadipti I've updated the images and pushed them into the main branch. You can check it out.
Co-authored-by: Jorge Orpinel <[email protected]>
@casperdcl I should directly commit it to the main branch, right? And, will the images load when I run the server locally? |
I've updated the link to Studio in the docs. It is now pointing to https://studio.iterative.ai Since this link isn't working currently, the tests are failing. Will re-check once @JIoJIaJIu updates https://github.com/iterative/viewer/issues/1891 |
@jorgeorpinel I've changed one image. For others (small images), hopefully the image borders will help for now.
yep :) And thanks for all your review comments. |
…tic folder in this repo
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.
Fixed a few minor things here and there. Thank you @tapadipti 🙏🏼 That's great content.
@jorgeorpinel I don't have merge access (got this "The base branch restricts merging to authorized users."). Can you merge this pls. |
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.
Couple comments on @iesahin last minute edits (thanks for the typo fixes)
For the next iteration. Although we'll probably rephrase lots of the text anyway. The Q now is what's the priority? Let's discuss during docs planning.
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.
Oops forgot to post them lol
content/docs/studio/create-view.md
Outdated
hover over the required repository and click on `Connect`. | ||
hover over the required repository and click `Connect`. |
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.
potayto, potahto 😬
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 checked some usage SO question for this :) It's potato :))
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.
🍠
view may be present in a data remote (cloud storage or another location | ||
outside of the Git repo). If you want to include such data in your views, then | ||
you will have to grant DVC Studio access to the data remote. | ||
view may be present in a <abbr>data remote</abbr> (cloud storage or another |
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.
No such tooltip (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.
content/docs/studio/index.md
Outdated
- **Run CI/CD for your ML projects on cloud resources of your choice without any | ||
new tools.** | ||
- **Run [CI/CD](https://en.wikipedia.org/wiki/CI/CD) for your ML projects on |
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.
Needed?
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.
In #2468 there is a checkbox that reads "The CI/CD part may need more explaining/linking" and the easiest way to link was Wikipedia. 😅 We can replace it with a concept page sometime I think.
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.
Yes, it's a good idea for now. I meant explaining CML runners I guess, or something related with CML. But not for this iteration. Anyway, we'll review when possible.
* #2468 Studio docs enhancements - initiated task * Changed the structure of the "teams" page + Added more details in the "experiments" page * Updated images + Some minor style changes * Text changes as per PR #2455 review comments * Added section on GH app installation + other small changes * Added clarification in the GitHub app section * Minor correction * Added a separate page for view settings * Clarified that DVC or non-DVC repos can be used with Studio * Added links to Studio in the DVC docs and banner * Added Studio icon in the banner * Apply suggestions from code review Co-authored-by: Casper da Costa-Luis <[email protected]> * Restyled by prettier * Added videos * Changed the intro of Studio docs as per Ivan's comment in the PR * Added DVC benefits, data remotes access details + some other changes * Small changes as per PR comments Co-authored-by: Casper da Costa-Luis <[email protected]> Co-authored-by: Restyled.io <[email protected]>
❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.
🐛 Please make sure to mention
Fix #issue
(if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.Please choose to allow us to edit your branch when creating the PR.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏