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

add gpu option #674

Merged
merged 6 commits into from
May 19, 2022
Merged

add gpu option #674

merged 6 commits into from
May 19, 2022

Conversation

vikimark
Copy link
Contributor

What does this changes

add gpu option on translate module

@pep8speaks
Copy link

pep8speaks commented May 18, 2022

Hello @vikimark! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-05-19 14:55:45 UTC

@cstorm125
Copy link
Member

LGTM. @wannaphong this is one of my Builders. Should we tell him to fix the PEP8 before merge?

@wannaphong
Copy link
Member

LGTM. @wannaphong this is one of my Builders. Should we tell him to fix the PEP8 before merge?

Yes, I think like you.

@wannaphong
Copy link
Member

I don't sure that It can working with GPU by use CUDA_VISIBLE_DEVICES to set available GPU. If it can use by use CUDA_VISIBLE_DEVICES to set available GPU, I think maybe we needs pull request to update docs not add gpu parameter.

Copy link
Member

@wannaphong wannaphong left a comment

Choose a reason for hiding this comment

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

This is my comment.

"""
:param str src_lang: source language
:param str target_lang: target language
:param str target_lang: target
Copy link
Member

Choose a reason for hiding this comment

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

Can you add use_gpu parameter to the docs?

@@ -51,7 +51,7 @@ class EnThTranslator:

Website: https://airesearch.in.th/releases/machine-translation-models/
"""
def __init__(self):
def __init__(self, use_gpu: bool):
Copy link
Member

Choose a reason for hiding this comment

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

Can you add use_gpu parameter to the docs?

@@ -51,7 +51,7 @@ class EnThTranslator:

Website: https://airesearch.in.th/releases/machine-translation-models/
"""
def __init__(self):
def __init__(self, use_gpu: bool):
Copy link
Member

Choose a reason for hiding this comment

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

I think it should has default is False.

@vikimark
Copy link
Contributor Author

I've resolved all your comments. And should I add docs to the init of each class?

@@ -7,6 +7,7 @@ class Translate:

:param str src_lang: source language
:param str target_lang: target language
:param bool use_gpu: load model to gpu
Copy link
Member

Choose a reason for hiding this comment

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

I think you should add default. example: (Default is False)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, added to all of them

@cstorm125 cstorm125 merged commit 91e15b4 into PyThaiNLP:dev May 19, 2022
@wannaphong wannaphong added this to the 3.1 milestone May 19, 2022
@coveralls
Copy link

coveralls commented May 19, 2022

Coverage Status

Coverage decreased (-0.2%) to 97.027% when pulling fcd9f4b on vikimark:add-translate-gpu-option into 3d2cd79 on PyThaiNLP:dev.

wannaphong added a commit that referenced this pull request Sep 1, 2022
See [3.1 Milestone](https://github.com/PyThaiNLP/pythainlp/milestone/16).

## What is new?
### Deprecation and other API changes
#687 Remove deprecated function
- pythainlp.word_vector; doesnt_match, get_model, most_similar_cosmul, sentence_vectorizer, similarity. use WordVector class instead
- pythainlp.util.delete_tone. use pythainlp.util.remove_tonemark instead
- Remove pythainlp.util.time_time. use pythainlp.util.time_to_thaiword instead
- pythainlp.tokenize.syllable_tokenize. use pythainlp.tokenize.subword_tokenize instead

### Name Entity Tagging
- #665 Add Thai-NNER `pythainlp.tag.NNER`
- #658 Add LST20NER onnx model. It is LST20NER model to onnx model from fine-turning by [WangchanBERTa model](https://huggingface.co/airesearch/wangchanberta-base-att-spm-uncased).

### Transliteration
- #659 Add ISO 11940 transliteration
- #660 Add Thai W2P v0.2
- #686 Add wunsen
- #694 Wunsen Mandarin and Japanese update

### PyThaiNLP Corpus downloader
- #656 Add support zip/tar.gz to download corpus

### Text normalization
- #673 Add a normalising rule for Lakkhangyao ๅ

### Translate
- #674 add gpu option

### Text summarize
- #679 Add mt5 cpe kmutt thai sentence sum

### Util
- #682 Add live-dead syllable classification
- #684 Add live dead syllable classify
- #690 Add tone detector

### Other
- #689 map NG tag to PART
- #691 Remove TinyDB as a dependency
- #692 Fix notifications that newer versions of corpora are available
@wannaphong wannaphong mentioned this pull request Sep 1, 2022
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.

5 participants