Skip to content

chore: Improve trtllm-serve import pattern - #3016

Closed
LinPoly wants to merge 1 commit into
NVIDIA:mainfrom
LinPoly:user/pengyunl/trtllm_serve_refactor
Closed

LinPoly wants to merge 1 commit into
NVIDIA:mainfrom
LinPoly:user/pengyunl/trtllm_serve_refactor

Conversation

@LinPoly

@LinPoly LinPoly commented Mar 24, 2025

Copy link
Copy Markdown
Collaborator

Replace with relative import.

Signed-off-by: Pengyun Lin <81065165+LinPoly@users.noreply.github.com>
@LinPoly
LinPoly requested a review from kaiyux March 24, 2025 09:00
@LinPoly LinPoly self-assigned this Mar 24, 2025
@chzblych

Copy link
Copy Markdown
Collaborator

/bot run

@niukuo

niukuo commented Mar 24, 2025

Copy link
Copy Markdown
Collaborator

PR_Github #276 [ run ] triggered by Bot

DisaggregatedParams,
ErrorResponse)
from tensorrt_llm.version import __version__ as VERSION
from ..executor import CppExecutorError

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think this is originally from

except CppExecutorError:
for handling error raised from Cpp code. By the way, I search the codebase and it seems like we don't raise such error anymore. Shall we extend the scope of this MR by removing all relative code?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry I'm still confused, you mean the previous way will raise errors?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It would handle the error from Cpp side,

except CppExecutorError as e:

@LinPoly LinPoly Mar 24, 2025 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Previously LLMAPI would raise such error if it ran into an error when calling Cpp functions, and trtllm-serve caught such error, treated it as unrecoverable and then stopped the server. But looks like we don't do similar thing right now.

else:
llm = LLM(**llm_args)

llm = LLM(**llm_args)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not suggested as offline discussed with @QiJune .

@niukuo

niukuo commented Mar 24, 2025

Copy link
Copy Markdown
Collaborator

PR_Github #276 [ run ] completed with state FAILURE
/LLM/main/L0_MergeRequest_PR pipeline #266 completed with status: 'FAILURE'

@kaiyux

kaiyux commented Mar 25, 2025

Copy link
Copy Markdown
Member

@LinPoly As discussed offline, maybe we can close this for now? I think we don't need to worry about the relative imports for now, if it sounds good to you.

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.

4 participants