Skip to content

Conversation

saptarshi-baseten
Copy link
Contributor

@saptarshi-baseten saptarshi-baseten commented Jul 17, 2025

🚀 What

  • This PR updates the TrussConfig validation to validate that the docker_server section has the expected fields depending on whether or not the deployment is gRPC
  • It also fixes test_model_wrapper.py::test_trt_llm_truss_missing_model_py which was failing in CI
image image

🔬 Testing

  • Checked that the appropriate errors are raised if docker_server is not present or has unexpected fields for gRPC deployments

Copy link

linear bot commented Jul 17, 2025

@saptarshi-baseten saptarshi-baseten marked this pull request as ready for review July 17, 2025 20:03
@saptarshi-baseten saptarshi-baseten changed the title Validate config.yaml fields depending on transport kind, attempt to fix integration test Validate config.yaml fields depending on transport kind, fix model wrapper test Jul 17, 2025
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you need to do getattr here? we've already checked that docker_server is not None, right?

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 docker_server is not None, but I think we need getattr to be able to check arbitrary fields in a loop like this (docker_server.server_port, etc.)

@saptarshi-baseten saptarshi-baseten force-pushed the saptarshibhattacherya/bt-15014-dont-allow-dev-models-for-grpc branch from 2cb1ca5 to 8228ec8 Compare July 23, 2025 20:56
apply_library_patches: bool = True
spec_version: str = "2.0"

DOCKER_SERVER_OPTIONAL_FIELDS: ClassVar[list[str]] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

these are only optional in the case of grpc right? can we rename this to be clearer

},
"Please define server_port, predict_endpoint, readiness_endpoint, and liveness_endpoint for docker_server",
id="invalid-http-missing-fields",
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be good to add a case for websockets

@spal1 spal1 self-assigned this Jul 25, 2025
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