-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Litgpt serve API #1299
Litgpt serve API #1299
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, added a few comments. We also need a test for this, feel free to inherit test code from litserve
since it's using clients etc
@aniketmaurya will be able to advise here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is sick. super excited about this landing. nice job.
It would be nice of you could review @lantiga and @aniketmaurya . Btw I am currently having problems with getting |
@@ -12,6 +12,7 @@ dependencies = [ | |||
"torch>=2.2.0", | |||
"lightning==2.3.0.dev20240328", | |||
"jsonargparse[signatures]>=4.27.6", | |||
"litserve==0.0.0.dev2", # imported by litgpt.deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rasbt Did you make this a required dependency (instead of an "all") purposefully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I remember correctly, I had problems with tugging the import so that it doesn't always gets called. I think that was due to how we use inheritance in
Line 18 in 57f0a61
class SimpleLitAPI(LitAPI): |
But I suppose one can define the class in a function to avoid the import at the top. But that would be ugly. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can follow this pattern:
litgpt/litgpt/data/prepare_starcoder.py
Lines 8 to 20 in 43c4432
from lightning_utilities.core.imports import RequirementCache | |
from litgpt import Tokenizer | |
from litgpt.utils import CLI | |
_LITDATA_AVAILABLE = RequirementCache("litdata") | |
if _LITDATA_AVAILABLE: | |
from litdata.processing.data_processor import DataChunkRecipe | |
else: | |
DataChunkRecipe = object | |
class StarcoderDataRecipe(DataChunkRecipe): |
And then inside the __init__()
, before calling super().__init__()
check that _LITSERVE_AVAILABLE
is True (this part is missing from the code I linked)
import shutil | ||
|
||
from lightning.fabric import seed_everything | ||
from fastapi.testclient import TestClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should specify this as a test dependency too
The LitGPT serve API. E.g., use as
and then send requests via