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

Update _toc.yml.in #2587

Closed
wants to merge 69 commits into from
Closed

Update _toc.yml.in #2587

wants to merge 69 commits into from

Conversation

Rmalavally
Copy link
Contributor

@Rmalavally Rmalavally commented Dec 6, 2023

DRAFT - TOC restructure

TOC restructure
docs/sphinx/_toc.yml.in Outdated Show resolved Hide resolved
samjwu
samjwu previously requested changes Dec 6, 2023
Copy link
Member

@samjwu samjwu left a 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

docs/sphinx/_toc.yml.in Outdated Show resolved Hide resolved
docs/sphinx/_toc.yml.in Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Rmalavally Rmalavally left a comment

Choose a reason for hiding this comment

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

Thanks, Sam.

@samjwu samjwu dismissed their stale review December 6, 2023 21:33

Fixed now

@saadrahim saadrahim marked this pull request as draft December 6, 2023 22:35
@bghimireamd
Copy link
Contributor

LGTM

@Rmalavally
Copy link
Contributor Author

LGTM

Thanks so much.

@Rmalavally Rmalavally marked this pull request as ready for review December 7, 2023 16:24
- file: MIOpen_Porting_Guide
- file: apireference
- caption: About
- caption: What is MIOpen?
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor

@LisaDelaney LisaDelaney Dec 8, 2023

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.

@LisaDelaney LisaDelaney marked this pull request as draft December 8, 2023 17:55
@LisaDelaney
Copy link
Contributor

these are no longer in the toc--is this intentional? Building MIOpen for Embedded Systems, Building the driver, Prerequisites

@Rmalavally
Copy link
Contributor Author

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.

@LisaDelaney
Copy link
Contributor

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.

@Rmalavally
Copy link
Contributor Author

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.

@LisaDelaney
Copy link
Contributor

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)

@Rmalavally
Copy link
Contributor Author

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
- file: reference/driver.rst
- caption: API reference
entries:
- file: reference/apireference.rst
Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

same goes for tutorials

Folder for MIOpen reference
@Rmalavally
Copy link
Contributor Author

@samjwu Using 'caption' breaks the build. Without the use of 'caption', it builds fine. Please advise.

Copy link
Collaborator

@JehandadKhan JehandadKhan left a 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].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the reference.

Copy link
Contributor Author

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.

Comment on lines 15 to 17
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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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.

@samjwu
Copy link
Member

samjwu commented Dec 12, 2023

@samjwu Using 'caption' breaks the build. Without the use of 'caption', it builds fine. Please advise.

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

@Rmalavally
Copy link
Contributor Author

Rmalavally commented Dec 12, 2023

@samjwu Using 'caption' breaks the build. Without the use of 'caption', it builds fine. Please advise.

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

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

@LisaDelaney
Copy link
Contributor

@samjwu Using 'caption' breaks the build. Without the use of 'caption', it builds fine. Please advise.

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

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
Copy link
Contributor

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).

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

@LisaDelaney
Copy link
Contributor

@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.

@Rmalavally
Copy link
Contributor Author

@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,

work on splitting up the individual files as needed (if applicable) and/or creating any new files you want (e.g. the "What is X?" page)
create any folders you need and put the appropriate files in the appropriate folders
get the toc file set up the way you want
update the index file(s) to align with the toc structure

@Rmalavally
Copy link
Contributor Author

@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,

work on splitting up the individual files as needed (if applicable) and/or creating any new files you want (e.g. the "What is X?" page)
create any folders you need and put the appropriate files in the appropriate folders
get the toc file set up the way you want
update the index file(s) to align with the toc structure

The ROCm .gitignore file has _toc.yml. Assume we will have _toc.yml.in in the .gitignore file. Please confirm.

@LisaDelaney
Copy link
Contributor

@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,

work on splitting up the individual files as needed (if applicable) and/or creating any new files you want (e.g. the "What is X?" page)
create any folders you need and put the appropriate files in the appropriate folders
get the toc file set up the way you want
update the index file(s) to align with the toc structure

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.

@Rmalavally
Copy link
Contributor Author

@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,

work on splitting up the individual files as needed (if applicable) and/or creating any new files you want (e.g. the "What is X?" page)
create any folders you need and put the appropriate files in the appropriate folders
get the toc file set up the way you want
update the index file(s) to align with the toc structure

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.

@junliume
Copy link
Collaborator

@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
Copy link
Contributor

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?

Copy link
Member

@samjwu samjwu left a 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

@LisaDelaney LisaDelaney closed this Mar 4, 2024
@junliume junliume deleted the Rmalavally-patch-1 branch June 25, 2024 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants