Skip to content

Conversation

@kyteinsky
Copy link
Contributor

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the translation service from using a context manager pattern to a persistent model loading approach, improving performance by loading the model once at thread initialization rather than for each translation request.

Key changes:

  • Introduced load_model() method to initialize tokenizer and translator as instance attributes
  • Removed the translate_context context manager and its resource cleanup logic
  • Modified translate() to use persistent self.tokenizer and self.translator instead of context manager resources

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
lib/main.py Adds call to service.load_model() in the task fetch thread to initialize the model before processing tasks
lib/Service.py Refactors from context manager pattern to persistent model loading with new load_model() method and updated translate() method to use instance attributes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Contributor

@oleksandr-nc oleksandr-nc left a comment

Choose a reason for hiding this comment

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

this PR needs to be rebased after a fix for ROCM PR

@kyteinsky
Copy link
Contributor Author

this PR needs to be rebased after a fix for ROCM PR

I was hoping merging this first would have solved this but probably not.
Will do.

Signed-off-by: Anupam Kumar <kyteinsky@gmail.com>
@kyteinsky kyteinsky force-pushed the feat/persistent-model branch from a157e0e to d676939 Compare November 7, 2025 12:51
Copy link
Contributor

@oleksandr-nc oleksandr-nc left a comment

Choose a reason for hiding this comment

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

changes looks ok (but did not test this, hopes it works good)

@kyteinsky
Copy link
Contributor Author

works on my machine (TM)
thanks for the review!

@kyteinsky kyteinsky merged commit 55240c1 into main Nov 7, 2025
6 checks passed
@kyteinsky kyteinsky deleted the feat/persistent-model branch November 7, 2025 13:11
@kyteinsky kyteinsky mentioned this pull request Nov 21, 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