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

Dvclive api reference #2632

Merged
merged 17 commits into from
Jul 16, 2021
Merged

Dvclive api reference #2632

merged 17 commits into from
Jul 16, 2021

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Jul 14, 2021

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ 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. 🙏

Closes iterative/dvclive#108

@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-api-ref-ndl8ie July 14, 2021 09:20 Inactive
@daavoo daavoo requested a review from dberenbaum July 14, 2021 14:54
Copy link
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

Great work! This looks really nice as a first draft. I'd also like to get someone from the docs team to review before merging (cc @jorgeorpinel).

We should also open a discussion on whether to include this in docstrings. I know there was already some discussion on autogenerating docs, but I think even if we have to duplicate text, it's worth considering for dvclive. IMO docstrings are generally accepted practice for a lot of ML libraries and could encourage contributions.

content/docs/dvclive/api-reference/init.md Outdated Show resolved Hide resolved
content/docs/dvclive/api-reference/init.md Outdated Show resolved Hide resolved
content/docs/dvclive/api-reference/log.md Outdated Show resolved Hide resolved
content/docs/dvclive/api-reference/next_step.md Outdated Show resolved Hide resolved
content/docs/dvclive/api-reference/next_step.md Outdated Show resolved Hide resolved
content/docs/dvclive/api-reference/next_step.md Outdated Show resolved Hide resolved
content/docs/dvclive/api-reference/log.md Outdated Show resolved Hide resolved
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-api-ref-ndl8ie July 14, 2021 21:14 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-api-ref-ndl8ie July 14, 2021 21:20 Inactive
@daavoo daavoo linked an issue Jul 14, 2021 that may be closed by this pull request
@daavoo
Copy link
Contributor Author

daavoo commented Jul 15, 2021

We should also open a discussion on whether to include this in docstrings. I know there was already some discussion on autogenerating docs, but I think even if we have to duplicate text, it's worth considering for dvclive. IMO docstrings are generally accepted practice for a lot of ML libraries and could encourage contributions.

I agree that it's worth to add the docstrings to dvclive even by copy-pasting content from here. I think I will wait for this P.R. to be merged and after that add the docstrings.

@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-api-ref-ndl8ie July 15, 2021 08:41 Inactive
@dberenbaum
Copy link
Contributor

@jorgeorpinel Should we assign a reviewer from the docs team?

## Description

It's usage is optional and focused on configuring the behavior of subsequent
calls to [`dvclive.log`](/doc/dvclive/api-reference/log) and
Copy link
Member

Choose a reason for hiding this comment

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

future improvement: we should create a ticket or improve auto-linker, to keep text clean from these internal links like we do in DVC API reference

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

some minor comments:

  • try to use more links, "abbr" tooltips, for people better understand certain concepts
  • write and prioritize the work on the ticket that would allow us to get rid of the explicit MD links to api reference - that should be very easy, similar to DVC.

@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-api-ref-ndl8ie July 16, 2021 19:46 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-api-ref-ndl8ie July 16, 2021 19:48 Inactive
@daavoo daavoo requested a review from shcheklein July 16, 2021 19:51
@shcheklein
Copy link
Member

Btw, on docstrings, I do think we need to include them (way simpler version though).

Also, as a next step, do we want to write some examples for these APIs?

@shcheklein shcheklein merged commit 4b2c927 into master Jul 16, 2021
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Hi 👋 just noticed this. I have lots of comments. Nothing super major but a few relevant questions and suggestions, and lots of minor ones (e.g. copy edits). A couple cosmetic ones 💅 too

Thanks

content/docs/dvclive/api-reference/index.md Show resolved Hide resolved
content/docs/dvclive/api-reference/init.md Show resolved Hide resolved
content/docs/dvclive/api-reference/init.md Show resolved Hide resolved
content/docs/dvclive/api-reference/init.md Show resolved Hide resolved
content/docs/dvclive/api-reference/init.md Show resolved Hide resolved
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jul 19, 2021

p.s. @daavoo I realize you may be focused on development so one option is to create a future docs improvement ticket linking to my #2632 (review) above. Maybe we can even work on it ourselves (docs team) — we can discuss in a planning meeting. Tanks

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jul 19, 2021

UPDATE: I guess could be part of iterative/dvclive/issues/86 but that's already a pretty long one 😅 kind of an epic?

@daavoo
Copy link
Contributor Author

daavoo commented Jul 19, 2021

p.s. @daavoo I realize you may be focused on development so one option is to create a future docs improvement ticket linking to my #2632 (review) above. Maybe we can even work on it ourselves (docs team) — we can discuss in a planning meeting. Tanks

I was already addressing your suggestions locally. My personal opinion is that, for dvclive, extending existing documentation would be more valuable at this point than development of new features and that is also what we have agreed at our planning.

@daavoo
Copy link
Contributor Author

daavoo commented Jul 19, 2021

UPDATE: I guess could be part of iterative/dvclive/issues/86 but that's already a pretty long one sweat_smile kind of an epic?

Yep. I tried to use the Github tasks feature to create new issues from the task list. Will create a new one for this API Reference updates.

@shcheklein
Copy link
Member

@jorgeorpinel to clarify, @daavoo 's scope of work includes writing docs/README and other product things, not only engineering. It's expected that he takes of the product holistically (similar to Casper's work on CML I think).

@jorgeorpinel
Copy link
Contributor

Got it, thanks guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Command/API Reference dvclive: clarify default config (init() is optional)
4 participants