Skip to content

Refine LLM getting started guide for uniformity, fix critical errors #2911

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

Closed

Conversation

GregoryComer
Copy link
Member

@GregoryComer GregoryComer commented Apr 7, 2024

Update the LLM getting started guide for uniform tone and tense. Informally following the Google developer documentation style guide: https://developers.google.com/style. Also, resolve a number of outstanding issues with incorrect or misleading documentation and steps.

For reference, here are links to the current and proposed LLM guide:
https://docs-preview.pytorch.org/pytorch/executorch/2911/llm/getting-started.html (proposed)
https://pytorch.org/executorch/main/llm/getting-started.html (live)

Copy link

pytorch-bot bot commented Apr 7, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/2911

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

❌ 1 New Failure

As of commit 678ba94 with merge base dc7e4d5 (image):

NEW FAILURE - The following job has failed:

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

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 7, 2024
@GregoryComer GregoryComer force-pushed the llm-doc-update-part-1 branch 2 times, most recently from ae6fe1a to b9cd8ff Compare April 8, 2024 10:39
@facebook-github-bot
Copy link
Contributor

@GregoryComer has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@GregoryComer has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@GregoryComer GregoryComer force-pushed the llm-doc-update-part-1 branch from b9cd8ff to 8c1bd79 Compare April 8, 2024 10:58
@GregoryComer GregoryComer changed the title Refine LLM getting started guide for uniformity Refine LLM getting started guide for uniformity, fix critical errors Apr 8, 2024
@@ -9,14 +9,22 @@
# Dependencies are defined in .pyproject.toml
if [[ -z $PYTHON_EXECUTABLE ]];
then
if [[ -z $CONDA_DEFAULT_ENV ]] || [[ $CONDA_DEFAULT_ENV == "base" ]];
if [[ -z $CONDA_DEFAULT_ENV ]] || [[ $CONDA_DEFAULT_ENV == "base" ]] || [[ ! -x "$(command -v python)" ]];
Copy link
Member Author

@GregoryComer GregoryComer Apr 8, 2024

Choose a reason for hiding this comment

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

Not sure how this happened, but heard of a case where python wasn't on the system path while in a conda env. Something to do with env/shell conflicts on mac? This change checks to make sure python actually exists. If not, default to python3, which seems to be the sanest option.

PIP_EXECUTABLE=pip
else
PIP_EXECUTABLE=pip3
fi
Copy link
Member Author

Choose a reason for hiding this comment

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

On mac when not under conda, pip may not exist. We should use pip3 when PYTHON_EXECUTABLE=python3.

@facebook-github-bot
Copy link
Contributor

@GregoryComer has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@byjlw byjlw left a comment

Choose a reason for hiding this comment

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

I added a couple of comments, but overall this is way better and feel free to merge

@@ -14,221 +14,308 @@

## Prerequisites

Let’s start by getting an ExecuTorch environment:
Copy link
Contributor

Choose a reason for hiding this comment

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

this is better. I like not having external links for instructions.

2. If you’re new to ExecuTorch follow [these steps](https://pytorch.org/executorch/main/getting-started-setup.html#set-up-your-environment) to set up your environment.
::::{tab-set}
:::{tab-item} conda
Instructions on installing miniconda can be [found here](https://docs.anaconda.com/free/miniconda).
Copy link
Contributor

Choose a reason for hiding this comment

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

can we eliminate the need for Conda.
Can you re-write this so that it uses env?

Copy link
Contributor

Choose a reason for hiding this comment

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

O looks like you have both options here. Perfect. I'd change the instructions just a tad so it calls out two different options up front before giving the conda instructions

```
string prompt = "Hello world!";
```
// Sample the next token from the logits.
Copy link
Contributor

Choose a reason for hiding this comment

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

so much better!

```

Also ExecuTorch provides different backend support for mobile acceleration. Simply call `to_backend()` with the specific backend partitioner on edge_manager during exportation. Take Xnnpack delegation as an example:
To export, run the script with `python export.py` (or python3, as appropriate for your environment). It will generate a `nanogpt.pte` file in the current directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be export_nanogpt.py?

et_program = edge_manager.to_executorch()
```
```cpp
// main.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to put them to the same piece?

et_program = edge_manager.to_executorch()
```
Finally, update the CMakeLists.txt to link the XNNPACK backend with the runner.

Finally, ensure that the runner links against the `xnnpack_backend` target in CMakeLists.txt.

```
add_executable(nanogpt_runner nanogpt_runner.cpp)
Copy link
Contributor

Choose a reason for hiding this comment

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

is it main.cpp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I updated the text here to be more clear.

```
vector<int64_t> outputs = generate(llm_model, tokens, sampler, /*target_output_length*/20);
https://github.com/GregoryComer/et-tutorials/blob/quantization/nanogpt/managed_tensor.h
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -348,15 +487,19 @@ target_link_libraries(
xnnpack_backend) # Link the XNNPACK backend
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's tell user exactly what's the diff?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also etdump is causing issue?

```
The `get_delegation_info()` method provides a summary of what happened to the model after the `to_backend()` call:

```python
from executorch.exir.backend.utils import get_delegation_info
Copy link
Contributor

Choose a reason for hiding this comment

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

which file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I'll update.

@GregoryComer GregoryComer force-pushed the llm-doc-update-part-1 branch 2 times, most recently from 9f75846 to 6996c48 Compare April 8, 2024 19:35
@facebook-github-bot
Copy link
Contributor

@GregoryComer has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@GregoryComer GregoryComer force-pushed the llm-doc-update-part-1 branch from 6996c48 to 6ba87b4 Compare April 8, 2024 21:00
@facebook-github-bot
Copy link
Contributor

@GregoryComer has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@GregoryComer has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@GregoryComer GregoryComer force-pushed the llm-doc-update-part-1 branch from 6ba87b4 to 678ba94 Compare April 8, 2024 21:22
@facebook-github-bot
Copy link
Contributor

@GregoryComer has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Olivia-liu pushed a commit to Olivia-liu/executorch-1 that referenced this pull request Apr 9, 2024
…ytorch#2911)

Summary:
Update the LLM getting started guide for uniform tone and tense. Informally following the Google developer documentation style guide: https://developers.google.com/style. Also, resolve a number of outstanding issues with incorrect or misleading documentation and steps.

For reference, here are links to the current and proposed LLM guide:
https://docs-preview.pytorch.org/pytorch/executorch/2911/llm/getting-started.html (proposed)
https://pytorch.org/executorch/main/llm/getting-started.html (live)

Pull Request resolved: pytorch#2911

Reviewed By: Gasoonjia, byjlw

Differential Revision: D55867181

Pulled By: GregoryComer
@facebook-github-bot
Copy link
Contributor

@GregoryComer merged this pull request in 01bac3d.

@GregoryComer
Copy link
Member Author

@pytorchbot cherry-pick --onto release/0.2 -c docs

pytorchbot pushed a commit that referenced this pull request Apr 9, 2024
…2911)

Summary:
Update the LLM getting started guide for uniform tone and tense. Informally following the Google developer documentation style guide: https://developers.google.com/style. Also, resolve a number of outstanding issues with incorrect or misleading documentation and steps.

For reference, here are links to the current and proposed LLM guide:
https://docs-preview.pytorch.org/pytorch/executorch/2911/llm/getting-started.html (proposed)
https://pytorch.org/executorch/main/llm/getting-started.html (live)

Pull Request resolved: #2911

Reviewed By: Gasoonjia, byjlw

Differential Revision: D55867181

Pulled By: GregoryComer

fbshipit-source-id: 5e865eaa4a0ae52845963b15c221a3d272431448
(cherry picked from commit 01bac3d)
@pytorchbot
Copy link
Collaborator

Cherry picking #2911

The cherry pick PR is at #2939

Details for Dev Infra team Raised by workflow job

mergennachin pushed a commit that referenced this pull request Apr 9, 2024
…2911)

Summary:
Update the LLM getting started guide for uniform tone and tense. Informally following the Google developer documentation style guide: https://developers.google.com/style. Also, resolve a number of outstanding issues with incorrect or misleading documentation and steps.

For reference, here are links to the current and proposed LLM guide:
https://docs-preview.pytorch.org/pytorch/executorch/2911/llm/getting-started.html (proposed)
https://pytorch.org/executorch/main/llm/getting-started.html (live)

Pull Request resolved: #2911

Reviewed By: Gasoonjia, byjlw

Differential Revision: D55867181

Pulled By: GregoryComer

fbshipit-source-id: 5e865eaa4a0ae52845963b15c221a3d272431448
(cherry picked from commit 01bac3d)
This was referenced Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants