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 readme #41

Merged
merged 4 commits into from
Apr 23, 2024
Merged

Refactor readme #41

merged 4 commits into from
Apr 23, 2024

Conversation

FanhaiLu1
Copy link
Contributor

This RP refactor readme to make it more clear. Here is are several principle for the refactor:

  1. Keep readme clean and simple
  2. Trade Jax and Pytorch inference stack equally
  3. Separate engine implementation README from JetStream README

@FanhaiLu1 FanhaiLu1 requested a review from vipannalla as a code owner April 18, 2024 17:04
Copy link
Contributor

@JoeZijunZhou JoeZijunZhou left a comment

Choose a reason for hiding this comment

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

I will let @gangji decide how we showcase the engines. If we need to make this change, shall we add the maxtext user guide to /docs/tutorials/online-inference-with-maxtext-engine.md. Similarly, we could also add /docs/tutorials/online-inference-with-pytorch-engine.md here.

@FanhaiLu1
Copy link
Contributor Author

I will let @gangji decide how we showcase the engines. If we need to make this change, shall we add the maxtext user guide to /docs/tutorials/online-inference-with-maxtext-engine.md. Similarly, we could also add /docs/tutorials/online-inference-with-pytorch-engine.md here.

Move maxtext to '/docs/tutorials/' sound reasonable. Ideally, I feel the user guide can be owned by maxtext.
For pytorch path, we have the readme in Jetstream-Pytorch repo, JetStream can refer it.

@gangji
Copy link
Collaborator

gangji commented Apr 22, 2024

In general, having a docs folder is a great idea. I am also a fan of keeping tests/docs close to the code. Also, having multiple source of truths might cause maintenance overhead. One solution is link to the https://cloud.google.com/tpu/docs/tutorials/LLM/jetstream. Open to suggestions. Thanks!

@qihqi
Copy link
Collaborator

qihqi commented Apr 22, 2024

Agree with Fanhai's change.

In principle:
Jetstream is a library that both maxtext and jetstream-pytorch depends.
Jetstream's docs should focus on "How to implement the Engine API and use jetstream itself"; i.e. if huggingface want to also implement the Engine API and serve their models what steps they follow etc. it should not focus how to use any particular impl of the engine, instead, it can link to those as reference examples.

@FanhaiLu1
Copy link
Contributor Author

In general, having a docs folder is a great idea. I am also a fan of keeping tests/docs close to the code. Also, having multiple source of truths might cause maintenance overhead. One solution is link to the https://cloud.google.com/tpu/docs/tutorials/LLM/jetstream. Open to suggestions. Thanks!

I feel we need two docs: 1: official doc for release version, this is the "https://cloud.google.com/tpu/docs/tutorials/LLM/jetstream" 2: Latest doc with current git code, this one we could use the git readme, the latest change also been updated to readme, but not to official doc as it's not release yet.

In current PR, I keep both git readme and the office doc. The overhead maintenance is minor, anyway engineer need to update the readme with change together if the change either brings new feature or current readme doesn't reflect the code (i.e. Zijun recent dns server address to ip change)

@gangji
Copy link
Collaborator

gangji commented Apr 22, 2024

cc @vipannalla.

@vipannalla
Copy link
Collaborator

Agree with others, we need separate doc folders. JetStream main README should focus on engine APIs and how to make it easy for anyone to write their own engine. We should just have links to Jetstream-Pytorch and MaxText as reference implemtations.

README.md Outdated

There are two engine implementation - Jax and Pytorch.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about "Currently, there are two reference engine implementations available -- one for Jax models and another for Pytorch models."

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, changed.

@@ -0,0 +1,289 @@
# JetStream MaxText Inference on v5e Cloud TPU VM User Guide
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should really be in MaxText repo, similar to pytorch one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file should really be in MaxText repo, similar to pytorch one.

We can do it later (after separation maxtext training vs inference ), there is no place I can put in maxtext repo right now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good.

@FanhaiLu1 FanhaiLu1 merged commit 21638b3 into AI-Hypercomputer:main Apr 23, 2024
3 checks passed
@FanhaiLu1 FanhaiLu1 deleted the doc branch April 23, 2024 05:48
jwyang-google pushed a commit that referenced this pull request May 6, 2024
* Reafactor readme

* update README

* Update README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants