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

Adding xeon_run_cpu.rst doc #2931

Merged
merged 17 commits into from
Jun 24, 2024
Merged

Adding xeon_run_cpu.rst doc #2931

merged 17 commits into from
Jun 24, 2024

Conversation

ZailiWang
Copy link
Contributor

@ZailiWang ZailiWang commented Jun 14, 2024

This PR is to add a user guide for torch.backends.xeon.run_cpu script.
https://github.com/pytorch/pytorch/blob/main/torch/backends/xeon/run_cpu.py

The content is same with pytorch PR pytorch/pytorch#128099
After some discussion we moved it to tutorials part.

@jingxu10 @jgong5 @svekars @kwen2501 @kurman
Please help review it.

cc @sekyondaMeta @svekars @kit1980 @brycebortree

Copy link

pytorch-bot bot commented Jun 14, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/tutorials/2931

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit 40b5da8 with merge base 3b97695 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot
Copy link
Contributor

Hi @ZailiWang!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@ZailiWang
Copy link
Contributor Author

Have sent mail to cla@meta.com for adding my account into Intel employee list.

Copy link
Contributor

@jgong5 jgong5 left a comment

Choose a reason for hiding this comment

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

Some minor revision suggested. Others LGTM.

recipes_source/recipes_index.rst Outdated Show resolved Hide resolved
recipes_source/recipes_index.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@svekars svekars left a comment

Choose a reason for hiding this comment

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

An editorial pass - thank you!

recipes_source/xeon_run_cpu.rst Outdated Show resolved Hide resolved
recipes_source/xeon_run_cpu.rst Outdated Show resolved Hide resolved
recipes_source/xeon_run_cpu.rst Outdated Show resolved Hide resolved
helping the users try out an optimal coordination of resource utilization for the specific workloads.

Prerequisites
-------------
Copy link
Contributor

Choose a reason for hiding this comment

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

please add the prerequisites

Copy link
Contributor Author

@ZailiWang ZailiWang Jun 18, 2024

Choose a reason for hiding this comment

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

Here by "prerequisites" I mean the users should install numactl/IntelOMP/TCMalloc/JeMalloc in prior to running the script, but seems misleading.
I changed the sections title to "Introduction of the Optimizations", and the following sub-titles are changed "Applying NUMA Access Control", "Using Intel(R) OpenMP Runtime Library" and "Choosing an Optimized Memory Allocator". Does it look clearer?

recipes_source/xeon_run_cpu.rst Outdated Show resolved Hide resolved
recipes_source/xeon_run_cpu.rst Outdated Show resolved Hide resolved
recipes_source/xeon_run_cpu.rst Outdated Show resolved Hide resolved
recipes_source/xeon_run_cpu.rst Outdated Show resolved Hide resolved
recipes_source/xeon_run_cpu.rst Outdated Show resolved Hide resolved
recipes_source/xeon_run_cpu.rst Show resolved Hide resolved
@svekars
Copy link
Contributor

svekars commented Jun 17, 2024

also, please sign the CLA

@ZailiWang
Copy link
Contributor Author

Hi @svekars thanks a lot for your efforts on correcting the expression in the article. I just pushed a commit per your comments, please help review again.

For CLA, I have sent mail to cla@meta.com for adding me into INTEL employee list, now waiting for approval from Intel POC.

What You Will Learn
-------------------

* How to utilize tools like ``numactl``, ``taskset``, Intel(R) OpenMP Runtime Library and optimized memory allocators such as ``TCMalloc`` and ``JeMalloc`` for enhanced performance.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* How to utilize tools like ``numactl``, ``taskset``, Intel(R) OpenMP Runtime Library and optimized memory allocators such as ``TCMalloc`` and ``JeMalloc`` for enhanced performance.
* How to utilize tools like ``numactl``, ``taskset``, Intel(R) OpenMP Runtime Library and
optimized memory allocators such as ``TCMalloc`` and ``JeMalloc`` for enhanced performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @svekars , it looks like we needn't separate these 2 sentences in 2 lines since they are bullets and have asterisks at the beginning.
With this change the output doc has 2 paragraphs for each bullet.
image

recipes_source/xeon_run_cpu.rst Outdated Show resolved Hide resolved
Comment on lines 204 to 205
+----------------------+------+---------------+-------------------------------------------------------------------------------------------------------------------------+
| knob | type | default value | help |
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why by the header row (Defaul value) in this table looks a bit weird:

Screenshot 2024-06-20 at 10 31 03 AM

I myself prefer list-tables for tables like this, For example:

.. list-table::
   :widths: 25 10 15 50
   :header-rows: 1
   * - knob
     - type
     - default value
     - help
   * - ``-h``, ``--help``
     - 
     - 
     - Show the help message and exit.
   * - ``-m``, ``--module``
     - 
     - 
     - Changes each process to interpret the launch script as a python module, executing with the same behavior as "python -m".
   * - ``--no-python``
     - bool
     - False
     - Do not prepend the program with "python" - just exec it directly. Useful when the script is not a Python script.
   * - ``--log-path``
     - str
     - ''
     - The log file directory. Default path is ``''``, which means disable logging to files.
   * - ``--log-file-prefix``
     - str
     - 'run'
     - log file name prefix.

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. I pushed a commit updating the formatting syntax of the tables per your advice.
Not deleting the old formatted tables for comparison.

@jingxu10
Copy link
Contributor

Hi @ZailiWang , pls address the comments above.

@ZailiWang
Copy link
Contributor Author

Hi @svekars seems I cannot trigger the building of the new commit for preview myself.
Would you help generate the preview html page again so that we can check the tables with new syntax is good. Also the adjustment of the column widths may be needed.

@svekars
Copy link
Contributor

svekars commented Jun 24, 2024

LGTM - please double check the formatting: https://docs-preview.pytorch.org/pytorch/tutorials/2931/recipes/xeon_run_cpu.html

@jingxu10
Copy link
Contributor

LGTM

@svekars svekars merged commit cbde504 into pytorch:main Jun 24, 2024
20 checks passed
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.

5 participants