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

Issue 1759 documentation #1760

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

bikash119
Copy link

This PR closes #1759

@r4victor
Copy link
Collaborator

r4victor commented Oct 2, 2024

I think the instructions on how to build the docs should be added to DEVELOPMENT.md since DOCUMENTATION.md
as of now duplicates it a lot. Also, we do not include dev dependencies in setup.py's [all]. Better to simply list them in requirements_docs.txt. The exact deps can be found in the docs.yaml github action:

pip install pillow cairosvg
sudo apt-get install -y libcairo2-dev libfreetype6-dev libffi-dev libjpeg-dev libpng-dev libz-dev
pip install mkdocs-material "mkdocs-material[imaging]" mkdocs-material-extensions mkdocs-redirects mkdocs-gen-files "mkdocstrings[python]" mkdocs-render-swagger-plugin --upgrade
pip install git+https://${{ secrets.GH_TOKEN }}@github.com/squidfunk/mkdocs-material-insiders.git
mkdocs build
- name: Deploy

@bikash119
Copy link
Author

Thank you @r4victor for reviewing the work. Will incorporate the feedback.

@bikash119
Copy link
Author

@r4victor : I have incorporated the feedback suggested.

@r4victor
Copy link
Collaborator

r4victor commented Oct 3, 2024

The deps list is missing pillow and cairosvg. Also, there should be an instruction for installing system deps on Linux/macOS.

I think It's more flexible to put docs deps in requirements_docs.txt so that the contributors don't need to install them if they are not working on docs.

@SamirPS
Copy link

SamirPS commented Oct 9, 2024

Why not using poetry instead of requirement.txt?

@peterschmidt85
Copy link
Contributor

@bikash119 I'm sorry but I don't see much value in this PR. The issue is important. Indeed it's unclear how to contribute to the docs and test the results. However, this PR doesn't help much.
Also, there is an issue. dstack uses MKDocs Insiders which requires GitHub token to install MKDocs Insiders. We cannot share our token. But still some guidance might be helpful I suppose. Currently, it's unclear for an external contributor how to contribute to the docs - regardless whether the user has or doesn't have a MKDocs Insiders token.

@bikash119
Copy link
Author

Thank you for your comments @peterschmidt85 . Based on comments from @r4victor , I think a external contributor can comment out the insider plugin in their local dev environment. As of today, its takes a little of debugging to get the mkdocs build to work. If we incorporate the feedbacks from @r4victor , external contributors can add documentations.
The CI workflow may be tweaked to verify if insider plugin is enabled for the build in PR request.
Please let me know if this sounds good. I will incorporate the feedbacks from @r4victor

@peterschmidt85
Copy link
Contributor

@bikash119 Basically we could add DOCUMENTATION.md under contributing and include clear instructions there on how to work with documentations - including how to set up the environment and test it.

@bikash119
Copy link
Author

@peterschmidt85 : In my first PR I had the exact same approach of adding DOCUMENTATION.md to contributing. After review comments from @r4victor which I agree as a it was duplication of DEVELOPMENT.md.

@peterschmidt85
Copy link
Contributor

@bikash119 We've just discussed it with @r4victor and agreed on the following:

  1. We don't change setup.py or requirements_dev.txt
  2. Instead, we introduce contributing/DOCUMENTATION.md but we have there two sections:
    2.1. Development setup (should mention that how to set up both with and without insider plugin)
    2.2. Working on the documentation (or something like that) that would elaborate a bit on how documentation is organized, how to test it locally, including how examples are organized (and when and how they are copied). Additionally, there should be a minimal section on the style but feel free to keep it blank – i can help with that.

@bikash119
Copy link
Author

Thank you @peterschmidt85 for detailed notes. Will work on it right now.

@bikash119
Copy link
Author

@bikash119 We've just discussed it with @r4victor and agreed on the following:

1. We don't change `setup.py` or `requirements_dev.txt`

2. Instead, we introduce `contributing/DOCUMENTATION.md` but we have there two sections:
   2.1. `Development setup` (should mention that how to set up both with and without insider plugin)
   2.2. `Working on the documentation` (or something like that) that would elaborate a bit on how documentation is organized, how to test it locally, including how examples are organized (and when and how they are copied). Additionally, there should be a minimal section on the style but feel free to keep it blank – i can help with that.

Here are my changes:

  • Kept the documentation dependencies in requirements_dev.txt. One of the feedback from @r4victor is to have different requirements_doc.txt for doc dependencies. May be we should keep it in requirements_dev.txt as most of the code changes will need some addition/ deletion to documentation. Please let me know if you think otherwise. Will create a new requirements_doc.txt
  • Created DOCUMENTATION.md which includes the steps for both development setup and documentation setup.
  • Deleted DEVELOPMENT.md

@peterschmidt85
Copy link
Contributor

@bikash119 You changed requirements_dev.txt and setup.py. It goes against what we agreed.
Also, you simply copied Development setup instructions from DEVELOPMENT.md to DOCUMENTATION.md instead what we agreed upon above. This doesn't make sense to me.

@bikash119
Copy link
Author

My bad @peterschmidt85 . Let me rework on it.

@bikash119
Copy link
Author

@peterschmidt85 , since this branch was messed up because of deletions and additions of new files. I have created a new PR : #1848.

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.

[Feature]: Make it easier to add documenation
4 participants