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

Add docstrings to public functions #767

Merged
merged 5 commits into from
Feb 8, 2024

Conversation

AlexandreKempf
Copy link
Contributor

@AlexandreKempf AlexandreKempf commented Feb 2, 2024

This PR fix this issue: #755

I was inspired by the dvc.org documentation for most arguments and descriptions. Let me know if you were hoping for something more technical.

I'm still struggling to find a nice description for cache, post_to_studio and save_dvc_exp. If you could enlighten me so I can finish this MR, that would be great.

I notice some missing type hints in the methods. I would love to correct those, but I'll do it on another PR.

@dberenbaum
Copy link
Contributor

Thanks @AlexandreKempf! The one question I have is how much to make the docstring match the actual docs. It looks like you copied a lot from the existing docs but also added or changed the language in some parts.

We aren't likely to auto-generate the docs from the docstrings anytime soon (the docs include some additional info that wouldn't fit well into docstrings, and the docs are integrated with the rest of the products and we don't want to take on a project to separately generate them with a different engine). However, if we don't have some guideline on how to keep them in sync, I worry we will end up having to review two sets of docs for every change we make.

@AlexandreKempf
Copy link
Contributor Author

Yeap I 100% agree @dberenbaum!
There will always be a problem with syncing documentation on change if we don't autogenerate one or the other.

Maybe we need to discuss who the readers for both docs (website and docstrings) are :)

My feeling (and the way I wrote this initial PR) is:
The website doc feels like it was designed for developers (technical terms, lots of code blocks), especially the API reference part we are discussing. My personal feeling is that the docstrings also target technical people. There are only two advantages over the web doc that I see:

  • when you don't have internet but DVClive is already installed. This means that there is no problem with having a duplicate between the web doc and the docstring (except for maintenance cost).
  • when you just want a quick glimpse at the doc from VScode. This means that the docstring can be much shorter than the web doc.

What would you suggest the readers are? Do you see additional use for docstrings? How would you change the current PR according to these points? Do you think there is a need for docstrings or correct and extensive typing is enough?

@AlexandreKempf
Copy link
Contributor Author

@dberenbaum I took your comment into account. Docstrings are now literally the same as our official doc!

src/dvclive/live.py Outdated Show resolved Hide resolved
AlexandreKempf

This comment was marked as off-topic.

@shcheklein
Copy link
Member

I think one of the next steps here is to have some good quick start snippets as well or links to them in docs string ... so that people can copy / paste basic stuff. Good work, thanks @AlexandreKempf !

@AlexandreKempf
Copy link
Contributor Author

@shcheklein 100% agree. Let's do that in another PR :) I'm creating an issue.

@dberenbaum
Copy link
Contributor

@AlexandreKempf Feel free to merge approved PRs. Do you have access to merge?

@AlexandreKempf AlexandreKempf merged commit ad74985 into main Feb 8, 2024
10 of 11 checks passed
@AlexandreKempf AlexandreKempf deleted the add-docstrings-to-public-functions branch February 8, 2024 18:43
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.

None yet

3 participants