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

Implemented frontend docs #3791

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

simveit
Copy link
Contributor

@simveit simveit commented Feb 22, 2025

Motivation

Rewrote frontend docs as jupyter notebook.

@shuaills shuaills self-requested a review February 22, 2025 21:30
Copy link
Collaborator

@shuaills shuaills left a comment

Choose a reason for hiding this comment

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

Great job

"from sglang.test.test_utils import is_in_ci\n",
"\n",
"if is_in_ci():\n",
" from patch import launch_server_cmd\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

CI failed, patch.py is not in proper place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i noticed. I will adjusted that and fix it tomorrow in better way.

Copy link
Contributor Author

@simveit simveit Feb 22, 2025

Choose a reason for hiding this comment

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

Actually I am not sure if there is an easy way without using python project or sys to make patch.py accessible in all the subfolders (both of which IMO would be overly complex and make the notebooks less readable...)
Maybe @zhaochenyang20 has a better idea.
Also the CI still failed, it seemed the !wget https://github.com/sgl-project/sglang/blob/main/test/lang/example_image.png?raw=true -O example_image.png i use didn't work because after the image got not recognized. Locally this worked of course. Any ideas about why that is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the current design of the patch. I mean including patch.py in sub-directories. I think for the image, you can refer to this https://docs.sglang.ai/backend/openai_api_vision.html

Copy link
Contributor Author

@simveit simveit Feb 23, 2025

Choose a reason for hiding this comment

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

ok i fixed the issue with wget.

import requests


def download_image(url, filename):
    response = requests.get(url)
    if response.status_code == 200:
        with open(filename, "wb") as f:
            f.write(response.content)
        print(f"Successfully downloaded {filename}")
    else:
        print(f"Failed to download image: Status code {response.status_code}")


image_url = "https://github.com/sgl-project/sglang/blob/main/test/lang/example_image.png?raw=true"
download_image(image_url, "example_image.png")

this code solved the problem

@simveit simveit force-pushed the feature/frontend-docs branch from 1b63fd2 to 5b908e6 Compare February 22, 2025 21:50
@simveit
Copy link
Contributor Author

simveit commented Feb 23, 2025

Before we merge this I will update the engine API now. I will put this also into this PR.

@simveit
Copy link
Contributor Author

simveit commented Feb 23, 2025

@shuaills @zhaochenyang20
I implemented vlm for engine doc, fixed mistake in example for engine with vlm (the script wants to access some tokenizer_manager attribute of engine which doesnt exist) and moved hidden_states extraction to examples.
For some reason the CI failed for the openai completions notebook which i didnt modify.
Shuai could you take a look at this please, i am not on computer now.
Screenshot 2025-02-23 at 17 12 02

@zhaochenyang20
Copy link
Collaborator

@simveit I rerun it and let's see what will happen. Are you agree with the docs? @shuaills I can merge it

@zhaochenyang20
Copy link
Collaborator

@simveit @shuaills I will wait approval from Shuai. But LGTM

Comment on lines 9 to 11
"import os\n",
"\n",
"os.environ[\"CUDA_VISIBLE_DEVICES\"] = \"1\""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, sorry this was for my local dev setup.

"image_token = conv.image_token\n",
"\n",
"# Convert image to bytes\n",
"image = Image.open(\"example_image.png\")\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use url here? So we don't need to manage file system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will adjust that to use the load_image function from utils.py (see below)

f.write(response.content)
print(f"Successfully downloaded {filename}")
else:
print(f"Failed to download image: Status code {response.status_code}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Download to file system may introduce some problems, can we use url instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe that will make make a large difference. Under the hood something like

def load_image(image_file: Union[str, bytes]):
    from PIL import Image

    image = image_size = None

    if isinstance(image_file, bytes):
        image = Image.open(BytesIO(image_file))
    elif image_file.startswith("http://") or image_file.startswith("https://"):
        timeout = int(os.getenv("REQUEST_TIMEOUT", "3"))
        response = requests.get(image_file, timeout=timeout)
        image = Image.open(BytesIO(response.content))
    elif image_file.lower().endswith(("png", "jpg", "jpeg", "webp", "gif")):
        image = Image.open(image_file)
    elif image_file.startswith("data:"):
        image_file = image_file.split(",")[1]
        image = Image.open(BytesIO(base64.b64decode(image_file)))
    elif image_file.startswith("video:"):
        image_file = image_file.replace("video:", "")
        image, image_size = decode_video_base64(image_file)
    elif isinstance(image_file, str):
        image = Image.open(BytesIO(base64.b64decode(image_file)))
    else:
        raise ValueError(f"Invalid image: {image}")

    return image, image_size

will be called under the hood if we provide an url.
A syntax you use in this notebook was tried to use before for the example and it didn't run. (threw error).

Copy link
Contributor Author

@simveit simveit Feb 24, 2025

Choose a reason for hiding this comment

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

But maybe we can use this function from above instead of custom code. this should be more elegant. i will adjust the code to that.

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 will have especially benefit we don't save anything as a file and I agree that this is little bit undesirable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The code mostly operates in memory, except when handling local file paths. But if we do open(filename, "wb") , it will accesse the file system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i will adjust to use to utils function from sglang code base from above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this @shuaills @zhaochenyang20 . The current implemetation is much easier and cleaner.

@simveit
Copy link
Contributor Author

simveit commented Feb 24, 2025

@zhaochenyang20 @shuaills i moved also the send request to the start and renamed section to "Getting started". Let me know if that is not what you intendend

@zhaochenyang20
Copy link
Collaborator

Sure. let shuai review this. And I will merge it after his approval.

@zhaochenyang20
Copy link
Collaborator

@shuaills Thanks!

@zhaochenyang20
Copy link
Collaborator

image

To me, i will remove VLM Inference and the get hidden state in these docs. But I will tell the users at the place I point to, "You can refer to these examples for VLM offline inference and getting hidden states."

For the getting hidden state examples, please make one, just following https://docs.sglang.ai/backend/offline_engine_api.html#Return-Hidden-States

@simveit

@simveit
Copy link
Contributor Author

simveit commented Feb 26, 2025

image To me, i will remove `VLM Inference` and the get hidden state in these docs. But I will tell the users at the place I point to, "You can refer to these examples for [VLM offline inference](https://github.com/sgl-project/sglang/blob/main/examples/runtime/engine/offline_batch_inference_vlm.py) and [getting hidden states](xxx)."

For the getting hidden state examples, please make one, just following https://docs.sglang.ai/backend/offline_engine_api.html#Return-Hidden-States

@simveit

I both included hidden states example and also fixed the vlm example for engine in this PR. let me remove the example from notebook later and refer to the links

docs/index.rst Outdated

start/install.md
start/send_request.ipynb
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you move send_reqeusts here, the patch would fail and send_request.ipynb will take large VRAM. But I think you can add one patch.py here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think we already have patch.py in start dir for this PR (see changed files)

@@ -0,0 +1,30 @@
import sglang as sgl
Copy link
Collaborator

Choose a reason for hiding this comment

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

add demonstration here. And saying that we are working on moving get_hidden_state to a sampling parameter rather than a server argument.

@simveit simveit force-pushed the feature/frontend-docs branch from 5fc29aa to afe2dd0 Compare February 26, 2025 20:02
@zhaochenyang20 zhaochenyang20 marked this pull request as ready for review February 26, 2025 20:09
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.

3 participants