-
Notifications
You must be signed in to change notification settings - Fork 40
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
Improve truss examples CI script #373
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.
Awesome, LGTM!
model_deployment = truss.push( | ||
target_directory, remote=REMOTE_NAME, trusted=True, publish=True | ||
) | ||
model_deployment.wait_for_active() |
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.
Nice, so this will block until the model comes up?
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.
yep, and fail fast as soon as there's a failure
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.
Could you import this from it's defining module instead? I.e. from truss.api...
? Forward-compatible with the separation of concerns in the truss package.
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.
Wait -- I thought from your PR we were leaving truss.push in the top-level? https://github.com/basetenlabs/truss/pull/1206/files#diff-92d69f0f5363f1d4e214eaf4a10c951888cf1a388ee43f4ed7cd57e720e837e1
There are several users using truss.push
in their CI jobs now, so removing this would be a pretty serious breaking change (we've documented this API) https://docs.baseten.co/truss-reference/python-sdk
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'm going to merge this for now since something seems quite broken with the current script (which calls the legacy predict endpoints), but let's discuss this further, want to make sure we're aligned (and can change if we have to)
Great quality of life improvement, thanks! |
model_deployment = truss.push( | ||
target_directory, remote=REMOTE_NAME, trusted=True, publish=True | ||
) | ||
model_deployment.wait_for_active() |
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.
Could you import this from it's defining module instead? I.e. from truss.api...
? Forward-compatible with the separation of concerns in the truss package.
Improves the truss examples to do the following:
Testing
Ran https://github.com/basetenlabs/truss-examples/actions/runs/11633933987/job/32400112601