-
Notifications
You must be signed in to change notification settings - Fork 38
overhaul elements of index.rst and inst.rst #497
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
Conversation
merging changes to highlight automatic BF16 conversion
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.
Pull Request Overview
This PR overhauls the documentation structure for index.rst and inst.rst to improve ease of use for developers. Key changes include reorganizing hardware configuration information, adding clearer installation instructions, and providing a structured development workflow overview.
- Moved hardware configuration and model support information from release notes to the main index page
- Enhanced installation instructions with specific links and PowerShell environment variable setup
- Added a 5-step development workflow overview for typical Ryzen AI usage
- Updated example titles and restructured documentation organization
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/relnotes_backup.rst | Created as backup containing original release notes content with hardware configurations |
| docs/relnotes.rst | Removed hardware configuration sections to focus on actual release notes |
| docs/inst.rst | Enhanced with detailed prerequisites table, PowerShell setup code, and clearer installation steps |
| docs/index.rst | Added comprehensive hardware support tables and development workflow overview |
| docs/getstartex.rst | Updated tutorial title and added more detailed explanations |
| docs/examples.rst | Simplified structure and updated example titles |
| docs/conf.py | Added sphinx_copybutton extension for code block copying |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| NPU | ||
| ~~~ | ||
|
|
||
| - :doc:`Getting Started Tutorial for INT8 models <getstartex>` - Uses a custom ResNet model to demonstrate: |
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.
Why remove "Getting Started" from the description?
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.
Can leave it in if you want. It is self-explanatory that it is for "getting started"
| - 2025 | ||
| - ☑️ | ||
| - | ||
| * - Ryzen Z2 |
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 don't believe this device is supported.
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.
Okay, can be removed then.
| **************** | ||
| Ryzen AI 1.6 Software runs on AMD processors outlined below. For a more detailed list of supported devices, refer to the `processor specifications <https://www.amd.com/en/products/specifications/processors.html>`_ page (scroll to the "AMD Ryzen™ AI" column toward the right side of the table, and select "Available" from the pull-down menu). Support for Linux is coming soon in Ryzen AI 1.6.1. | ||
|
|
||
| .. list-table:: Supported Ryzen AI Processor Configurations |
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 table will only grow and will be hard to maintain as we add support for more platforms. It's redundant with the https://www.amd.com/en/products/specifications/processors.html page. And we risk creating inconsistencies. Case in point, see the comment about Z2 below.
I recommend we simply link to the official processor specification page.
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 processors page you linked is very hard to navigate. You have to scroll across multiple screens just to find the info you need. It would be nice to have some kind of compatibility table right on the docs site. It doesn't have to be as broad as what I've described. But I am confused when there is no table to summarize what processor goes with what abbreviation throughout the docs. People only know the "STX", "KRK" and other abbreviations internal to AMD.
| - GPU | ||
| - NPU | ||
| - Hybrid (NPU + iGPU) | ||
| * - Ryzen AI 300 |
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 support LLMs on all STX and KRK platforms, not all Ryzen AI 300. The first column in this table is not needed and should be removed.
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 whole point is to relate the processor series name to the codename here. It is confusing as to what the common name people refer to it as, versus the codename.
| .. list-table:: | ||
| :header-rows: 1 | ||
|
|
||
| * - Model Type |
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.
Why mention CPU and GPU for LLMs and not for other models? BF16 models can run on CPU and GPU on PHX/HPT.
The way LLMs and CNN/NLPs are presented in inconsistent. It would be preferable to find a common way to presenting the information.
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.
Okay, open to your suggestions as to a better way to present it; but I think a table does a good job of communicating the information.
| ************************* | ||
|
|
||
| The Ryzen AI development flow does not require any modifications to the existing model training processes and methods. The pre-trained model can be used as the starting point of the Ryzen AI flow. | ||
| A typical Ryzen AI flow might look like the following: |
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 is accurate for CNNs, but not for BF16 NLPs (no quantization step) or LLMs (OGA flow).
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 adding "for CNNs" is fine.
| - 2022 with `Desktop Development with C++` checked | ||
| * - `cmake <https://cmake.org/download/>`_ | ||
| - >= 3.26 | ||
| * - `Python (Miniforge preferred) <https://conda-forge.org/download/>`_ |
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.
Should we really say "Miniforge preferred"?
Internally to AMD, we need to use Miniforge. But other companies may have different requirements.
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.
Can remove that statement. The point here was to add links for suggestions of where to get the packages they need. It's fine to link Conda / Anaconda as well, but I thought it was better to have links for developers to get what they need, rather than having no links and just having people to go off and find what they need.
| - Miniforge: ensure that the following path is set in the System PATH variable: ``path\to\miniforge3\condabin`` or ``path\to\miniforge3\Scripts\`` or ``path\to\miniforge3\`` (The System PATH variable should be set in the *System Variables* section of the *Environment Variables* window). | ||
| $existingPath = [System.Environment]::GetEnvironmentVariable('Path', 'Machine') | ||
| .. code-block:: powershell |
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.
Why not put all 3 lines in the same code block?
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.
Sure. The point here was to add actual Powershell code, rather than bash code when people are working on Windows.
| .. code-block:: powershell | ||
| $newPaths = "C:\Users\<user>\miniforge3\Scripts;C:\Users\<user>\miniforge3\condabin" |
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 will only work for miniforge. If people have Anaconda or Miniconda, this will not work.
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.
Can just be resolved by saying "May need to adjust paths according to your conda installation."
ThomasXilinx
left a 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.
Some of the proposed changes need more discussion
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Added comments to the changes. Thanks for the feedback. |
Overhaul of
index.rstandinst.rstfor ease of use for developers