-
Notifications
You must be signed in to change notification settings - Fork 230
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
Update _toc.yml.in #2587
Update _toc.yml.in #2587
Conversation
TOC restructure
Changed title to caption
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.
The table of contents has a very specific format it accepts
A good example is the ROCm TOC: https://github.com/RadeonOpenCompute/ROCm/blob/develop/docs/sphinx/_toc.yml.in
Corrected white space for - caption
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.
Thanks, Sam.
LGTM |
Thanks so much. |
- file: MIOpen_Porting_Guide | ||
- file: apireference | ||
- caption: About | ||
- caption: What is MIOpen? |
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.
this should not be a heading. it should be an intro page that describes the project.
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 are still discussing the content for the Intro page. For now, we have reorganized the ToC on what's available and accurate. We have also removed the reference to release notes, as it is obsolete.
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.
then we need to wait until we have a page to link to. It's confusing to have a heading represent a placeholder for a future page. A heading (='caption' in the toc) should only be there to represent pages that fit into that heading.
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.
That's a decision we must make. Working with the team.
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.
you can use the README file to get a little general info for a 'What is ..?' page until the team gets a chance to add to it. this page doesn't need to super long or elaborate, just has to describe what the project is/does.
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.
Thanks, Lisa. Thanks so much for your help and comments.
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.
where is the API ref heading? you seem to have removed the entire former API ref section and there is no organization to the existing pages
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.
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.
look at the build--it's not rendering, so you need to fix the code.
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.
Yep. It's a work in progress. Won't render 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.
you haven't added any of the Doxygen content (=the API ref)
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.
- file: reference/apireference.rst is what I have from them. Let's hold off until the structure is finalized.
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.
Doxygen builds content automatically from the header files (which contains the code/text/content for the API). The team does does not supply the API textual content. This content is build into the "Doxygen" folder--this is what you need to include in the toc in order to incorporate the API ref.
these are no longer in the toc--is this intentional? Building MIOpen for Embedded Systems, Building the driver, Prerequisites |
The restructure is based on their approval. I am not an expert to make this call. Let me finalize the content and get back to the team. |
We can't publish their documentation without the API ref. The API ref is the most important part. |
Sure. I am aware. We used to have MIOpen on RTD before with other docs. Let's not rush without ensuring we are doing it correctly. |
the landing page (index) is not ready. This PR needs more than toc edits before it can be merged...(at a minimum, add the updated index and what is pages) |
I agree. Even with the minimum updates, it may not be consistent. Let's evaluate the implications of making this publicly available for this release. |
Removed citation.rst based on customer request https://ontrack-internal.amd.com/browse/SWDEV-416591
docs/sphinx/_toc.yml.in
Outdated
- file: reference/driver.rst | ||
- caption: API reference | ||
entries: | ||
- file: reference/apireference.rst |
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.
the files here in the TOC are relative to the docs/
folder
for this to render correctly, apireference.rst
needs to go into a folder named reference
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.
same goes for tutorials
Updates
Folder for MIOpen reference
@samjwu Using 'caption' breaks the build. Without the use of 'caption', it builds fine. Please 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.
Minor edits
.. meta:: | ||
:description: MIOpen is AMD’s deep learning primitives library, which provides highly optimized, and hand-tuned implementations of | ||
different operators such as convolution, batch normalization, pooling, softmax, activation and layers for Recurrent Neural | ||
Networks (RNNs), used in both training and inference [9]. |
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.
Please remove the reference.
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 did. Let me resend the PR. Here's the final text,
MIOpen is AMD’s deep learning primitives library, which provides highly optimized and hand-tuned implementations of different operators such as convolution, batch normalization, pooling, softmax, activation and layers for Recurrent Neural Networks (RNNs), used in both training and inference. Moreover, MIOpen is fully open source, including all its GPU kernels, complementing AMD’s open source ROCm stack. MIOpen is the first to extend the open source advantage into GPU vendor libraries thereby, continuing to embark on the same ethos as the deep learning community.
docs/tutorials/what-is-MIOpen.rst
Outdated
Networks (RNNs), used in both training and inference. Moreover, MIOpen is fully open-source including all its | ||
GPU kernels; complementing AMD’s open source ROCm stack [10]. MIOpen is the first to extend the open source | ||
advantage into GPU vendor libraries thereby, continuing to embark on the same ethos as the deep learning community. |
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.
Networks (RNNs), used in both training and inference. Moreover, MIOpen is fully open-source including all its | |
GPU kernels; complementing AMD’s open source ROCm stack [10]. MIOpen is the first to extend the open source | |
advantage into GPU vendor libraries thereby, continuing to embark on the same ethos as the deep learning community. | |
Networks (RNNs), used in both training and inference. |
Please refer to this version of the toc. It has the correct formatting to build the documentation https://github.com/ROCm/MIOpen/blob/cac7a8872707c117d88cb56993470c7f371809cf/docs/sphinx/_toc.yml.in |
Minor tweaks
If I use hipSPARSElt as the template, then a title is required for each file. The updated ToC with titles builds fine as long as caption is not used. https://github.com/ROCm/hipSPARSELt/blob/develop/docs/sphinx/_toc.yml.in |
A title is not required...but I like to include it because, if you don't, it uses the title (H1) from within the file, which is often too long for a toc. You can find more info on correct syntax here: https://sphinx-external-toc.readthedocs.io/en/latest/user_guide/sphinx.html#basic-structure. Note that YAML is very particular about spacing/tabs. |
- caption: API reference | ||
subtrees: | ||
- entries | ||
- file: reference/apireference.rst |
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.
You're referencing a lot of files/folders that don't exit...and need to include the API ref from the doxygen folder (see my previous comment).
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.
Bibek and I added the API ref from the Doxygen folder. @bghimireamd Please 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.
Exit? or Exist? We have reused all existing and original files. Sam recommended we create the appropriate folders, and as suggested, we used hipSPARSElt as the template. Should we remove the titles and keep it simple for 6.0?
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.
that was a typo: I meant exist.
@Rmalavally I just noticed there is no gitignore file in this repo--can you create one (it will live in the root directory)? We don't need the build/doxygen folder/other build artifacts being tracked in the GitHub repo. You can use the ROCm repo gitignore as a reference. |
Let me check. Could you also update the doc guidelines accordingly? For now, we have,
|
The ROCm .gitignore file has _toc.yml. Assume we will have _toc.yml.in in the .gitignore file. Please confirm. |
No, _toc.yml is an autogenerated file (a copy of _toc.yml.in); _toc.yml.in should not be in the gitignore file. |
Ok. Good to know. Thanks for clarifying. |
@Rmalavally this PR is still in draft state, if we are pursuing it to merging, please update it and resolve all the conflicts above. Thanks! |
|
||
python: | ||
install: | ||
- requirements: docs/sphinx/requirements.txt | ||
|
||
build: | ||
os: ubuntu-22.04 |
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.
@samjwu is this change correct?
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.
.readthedocs.yaml
should not need to be changed. It is for the ReadtheDocs site build configurations
DRAFT - TOC restructure