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

Stop copying truss server code over for "trussless" #1189

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

Conversation

tianshuc0731
Copy link
Contributor

@tianshuc0731 tianshuc0731 commented Oct 12, 2024

🚀 What

We caught some bugs in trussless custom server in real life when customers try to start their server using python main.py as start command
https://basetenlabs.slack.com/archives/C037X7HFAE4/p1728672357619419?thread_ts=1728574438.184139&cid=C037X7HFAE4

💻 How

  • stop copying over truss code to container in "trussless" mode
  • stop overriding WORKDIR in "trussless" mode
  • remove torch dependency from trussless test case for faster image building and test running
  • change trussless test case to use python main.py to start fastapi server, so we can catch similar type of bugs in the future
  • some trussless files renaming

🔬 Testing

Passed integration test
https://github.com/basetenlabs/truss/actions/runs/11353710099

Copy link
Collaborator

@squidarth squidarth left a comment

Choose a reason for hiding this comment

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

lgtm.

Might also be worth trying out dhruv's config before merging

@tianshuc0731
Copy link
Contributor Author

lgtm.

Might also be worth trying out dhruv's config before merging

@dsingal0 can you try context image "v0.9.44-rc" to verify it can resolve the issue?

@squidarth
Copy link
Collaborator

@tianshuc0731 I also think that we might need to merge #1187 to properly test dhruv's changes. it lgtm

@tianshuc0731
Copy link
Contributor Author

@tianshuc0731 I also think that we might need to merge #1187 to properly test dhruv's changes. it lgtm

Merged the change, @dsingal0 please test this new image "v0.9.44-rc2"

pyproject.toml Outdated Show resolved Hide resolved
truss/truss_handle.py Show resolved Hide resolved
truss/truss_handle.py Outdated Show resolved Hide resolved
@@ -106,7 +108,7 @@ RUN mkdir -p {{ supervisor_log_dir }}
COPY supervisord.conf {{ supervisor_config_path }}
ENV SUPERVISOR_SERVER_URL="{{ supervisor_server_url }}"
ENV SERVER_START_CMD="supervisord -c {{ supervisor_config_path }}"
ENTRYPOINT ["/usr/local/bin/supervisord", "-c", "{{ supervisor_config_path }}"]
ENTRYPOINT ["supervisord", "-c", "{{ supervisor_config_path }}"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

one non-blocking comment, since this resulted in an actual bug/issue, would be awesome if we could also have an integration test for this, don't know how easy that would be to put together

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No easy way on top of my mind, maybe what we want is to have a different testing docker image which installs supervisord in a different path? Any better ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah maybe something like that. no need to block on this then

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