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.

Sign up for GitHub

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

Jump to bottom

add ct2 server parameters #2088

Closed
wants to merge 2 commits into from
Closed

add ct2 server parameters #2088

wants to merge 2 commits into from

Conversation

900groove
Copy link

fix: #2001

This pull request will allow to set the server specs for CTranslate2

Experiment

Setting

server_conf

{
    "models_root": "./available_models",
    "models": [
        {
            "id": 100,
            "ct2_model": "ende_ctranslate2/",
            "model": "ende_ctranslate2/",
            "timeout": 600,
            "device": "cpu",
            "on_timeout": "to_cpu",
            "load": true,
            "tokenizer": {
                "type": "sentencepiece",
                "model": "sentencepiece.model"
            },
            "inter_threads": 1,
            "intra_threads": 1,
            "compute_type": "default",
            "opt": {
                "beam_size": 5
            }
        }
    ]
}

Data

Performance

Process evaluation by changing inter_threads and intra_threads

inter_threads intra_threads time_avg[sec]
1 1 0.55
1 8 0.29
8 1 0.57
8 8 1.82

@francoishernandez
Copy link
Member

Hi there,
Sorry this went a bit under my radar.
Thanks for you contribution, but wouldn't it be better/cleaner to pass some ct2_kwargs dict as kwargs of the CT2 Translator constructor to allow all possible parameters in a transparent way, rather than manually adding each of them in the code?

@900groove
Copy link
Author

@francoishernandez
Thank you for your comment!
I'm not entirely sure I understand your comment. Is setting parameters in a way other than kwargs intended to look like environment variables?

@francoishernandez
Copy link
Member

I mean that we could do something similar to what is done for pyonmttok, where all the tokenizer kwargs are in a "tokenizer" key in the config, and then passed automatically to the constructor:

OpenNMT-py/onmt/translate/translation_server.py

Lines 691 to 697 in 7e8c7c2

tokenizer_params = dict(tokenizer_opt["params"])
for key, value in tokenizer_opt["params"].items():
if key.endswith("path"):
tokenizer_params[key] = os.path.join(
self.model_root, value)
tokenizer = pyonmttok.Tokenizer(mode,
**tokenizer_params)

@funboarder13920
Copy link
Collaborator

I implemented the ct2 params in #2104 using two conf dictionaries to allow all ct2 params to be forwarded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve CTranslate2 wrapping in translation_server
3 participants
@900groove @francoishernandez @funboarder13920