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

Refactor and Fix the Readme #563

Merged
merged 16 commits into from
May 6, 2024
Merged

Refactor and Fix the Readme #563

merged 16 commits into from
May 6, 2024

Conversation

byjlw
Copy link
Contributor

@byjlw byjlw commented Apr 29, 2024

Readme updated to match the new structure and feedback from Soumith
Sections should be more atomic meaning a user can go straight to the mobile section not get stuck even if they didn't read from start to finish.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Apr 29, 2024
@mikekgfb mikekgfb self-requested a review April 29, 2024 21:48
Copy link
Contributor

@mikekgfb mikekgfb left a comment

Choose a reason for hiding this comment

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

Detailed feedback as issues, as I'm working thru the README.

Update README.md
README.md Outdated
@@ -161,11 +161,11 @@ This will download the ExecuTorch repo to ./et-build/src and install various Exe
```
export TORCHCHAT_ROOT=${PWD}
export ENABLE_ET_PYBIND=true
./scripts/install_et.sh $ENABLE_ET_PYBIND
./scripts/install_et.sh --pybind
Copy link
Contributor

Choose a reason for hiding this comment

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

Supposedly we don't need an option here, we have a PR from @metascroy

README.md Outdated
@@ -63,51 +85,19 @@ with `python3 torchchat.py remove llama3`.
* **Access to model is restricted and you are not in the authorized list. Visit \[link\] to ask for access**:
Some models require an additional step to access. Follow the link to fill out the request form on HuggingFace.

## What can you do with torchchat?
Copy link
Contributor

Choose a reason for hiding this comment

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

The installation instructions do not work. x-ref: #557

@byjlw byjlw marked this pull request as ready for review May 2, 2024 22:44
README.md Outdated
@@ -89,7 +83,12 @@ View available models with:
python3 torchchat.py list
```

You can also remove downloaded models with the remove command: `python3 torchchat.py remove llama3`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit on this one is that I'd actually like them not to run it, so actually better to have inline?

Copy link
Contributor

@mikekgfb mikekgfb May 4, 2024

Choose a reason for hiding this comment

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

+1

Yes, I commented the same and edited the command in the README.md, togther with many others. And no, I don't deal with branches. Just submit your stuffrather than having it in a side branch and just let it drop from heaven, like a pino in a cartoon.

README.md Outdated
NOTE: We use `--quantize config/data/cuda.json` to quantize the llama3 model to reduce model size and improve performance for on-device use cases.
python3 torchchat.py generate llama3 --dso-path exportedModels/llama3.so --prompt "Hello my name is"
```
NOTE: If you're machine has cuda add this flag for performance `--quantize config/data/cuda.json`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: has cuda -> has CUDA

Copy link
Contributor

Choose a reason for hiding this comment

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

Also: "you're machine" => "your machine"

README.md Outdated
@@ -126,16 +125,17 @@ Enter some text in the input box, then hit the enter key or click the “SEND”
### AOTI (AOT Inductor)
AOT compiles models before execution for faster inference

The following example exports and executes the Llama3 8B Instruct model
The following example exports and executes the Llama3 8B Instruct model. (The first command performs the actual export, the second command loads the exported model into the Python interface to enable users to test the exported model.)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to add parenthesis on that second sentence?

@byjlw byjlw merged commit f54680e into main May 6, 2024
34 checks passed
malfet pushed a commit that referenced this pull request Jul 17, 2024
* refactoring the readme

* continued refining

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more refining

* Update README.md

Update README.md

* move the discaimer down

* remove torchtune from main readme
Fix pathing issues for runner commands

* don't use pybindings for et setup

---------

Co-authored-by: Michael Gschwind <[email protected]>
malfet pushed a commit that referenced this pull request Jul 17, 2024
* refactoring the readme

* continued refining

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more refining

* Update README.md

Update README.md

* move the discaimer down

* remove torchtune from main readme
Fix pathing issues for runner commands

* don't use pybindings for et setup

---------

Co-authored-by: Michael Gschwind <[email protected]>
malfet pushed a commit that referenced this pull request Jul 17, 2024
* refactoring the readme

* continued refining

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more refining

* Update README.md

Update README.md

* move the discaimer down

* remove torchtune from main readme
Fix pathing issues for runner commands

* don't use pybindings for et setup

---------

Co-authored-by: Michael Gschwind <[email protected]>
malfet pushed a commit that referenced this pull request Jul 17, 2024
* refactoring the readme

* continued refining

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more refining

* Update README.md

Update README.md

* move the discaimer down

* remove torchtune from main readme
Fix pathing issues for runner commands

* don't use pybindings for et setup

---------

Co-authored-by: Michael Gschwind <[email protected]>
malfet pushed a commit that referenced this pull request Jul 17, 2024
* refactoring the readme

* continued refining

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more refining

* Update README.md

Update README.md

* move the discaimer down

* remove torchtune from main readme
Fix pathing issues for runner commands

* don't use pybindings for et setup

---------

Co-authored-by: Michael Gschwind <[email protected]>
malfet pushed a commit that referenced this pull request Jul 17, 2024
* refactoring the readme

* continued refining

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more cleanup

* more refining

* Update README.md

Update README.md

* move the discaimer down

* remove torchtune from main readme
Fix pathing issues for runner commands

* don't use pybindings for et setup

---------

Co-authored-by: Michael Gschwind <[email protected]>
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 Meta Open Source bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants