Collapse Service #62

jerinphilip · 2021-03-22T16:34:43Z

For #61.

Collapses Service to a single class, we have ifdefs back.
Service has a constructor which directly takes in a string config and calls second constructor after parsing string into options. This is now connected to TranslationModel to ensure tests cover this all constructors.
WASM builds are succeeding.
Doxygen compatible documentation/comments on Service.
Passes bergamot-translator-tests

There are untested code-paths (NOT WITH_PTHREADS and not wasm) not covered currently by bergamot-regression-tests.

Ugliest ifdefs I have done in my life.

To reduce tax for bytebuffer loads, initialize batchtranslator only at one place.

src/translator/CMakeLists.txt

src/translator/service.h

src/translator/service.cpp

Sane platform (non WASM) is default. This truly only hide-threads from compilation path and not switch unswitch pthreads (-lpthread).

src/translator/service.cpp

jerinphilip · 2021-03-23T01:25:27Z

WASM builds are now succeeding (once again, after breaking with WASM_HIDE_THREADS).

Bergamot translator regression-tests:

Simple tests: bergamot-translator-app (single thread), service-cli (multi-thread) - PASS

jphilip@var:~/code/bergamot-translator-test-scripts$ MARIAN=/mnt/Storage/jphilip/bergamot-build/ ./run_brt.sh
[03/23/2021 01:07:35] Running on var as process 64934
[03/23/2021 01:07:35] Using Marian binary: /mnt/Storage/jphilip/bergamot-build/marian
[03/23/2021 01:07:35] Version: v1.9.56 370fdb5a 2021-03-10 17:12:37 +0000
[03/23/2021 01:07:35] Build type: Release
[03/23/2021 01:07:35] Using compiler: /usr/bin/c++
[03/23/2021 01:07:35] Using MKL: COMPILE_CPU=on
[03/23/2021 01:07:35] Using CUDNN:
[03/23/2021 01:07:35] Using SentencePiece: USE_SENTENCEPIECE=ON
[03/23/2021 01:07:35] Using FBGEMM:
[03/23/2021 01:07:35] Unit tests:
[03/23/2021 01:07:35] Using CUDA visible devices:
[03/23/2021 01:07:35] Using number of GPU devices: 1
[03/23/2021 01:07:35] Using time out: 5m
[03/23/2021 01:07:35] Checking directory: tests
[03/23/2021 01:07:35] Checking directory: tests/basic
[03/23/2021 01:07:35] Running tests/basic/test_bergamot_translator_app_intgemm_8bit.cpu-threads.0.sh ... OK
[03/23/2021 01:07:37] Test took 00:00:1.571s
[03/23/2021 01:07:37] Running tests/basic/test_service-cli_intgemm_8bit.cpu-threads.4.sh ... OK
[03/23/2021 01:07:40] Test took 00:00:2.875s
---------------------
Ran 2 tests in 00:00:4.539s, 2 passed, 0 skipped, 0 failed

Speed test: 48 threads, 1M lines - PASS, no segfaults.

jphilip@var:~/code/bergamot-translator-test-scripts$ MARIAN=/mnt/Storage/jphilip/bergamot-build/ TIMEOUT=45m BRT_THREADS=48 BRT_EXPECTED_MAXTIME=500 ./run_brt.sh speed-tests/test_wngt20_perf.sh

[03/23/2021 01:07:43] Running on var as process 65087
[03/23/2021 01:07:43] Using Marian binary: /mnt/Storage/jphilip/bergamot-build/marian
[03/23/2021 01:07:43] Version: v1.9.56 370fdb5a 2021-03-10 17:12:37 +0000
[03/23/2021 01:07:43] Build type: Release
[03/23/2021 01:07:43] Using compiler: /usr/bin/c++
[03/23/2021 01:07:43] Using MKL: COMPILE_CPU=on
[03/23/2021 01:07:43] Using CUDNN:
[03/23/2021 01:07:43] Using SentencePiece: USE_SENTENCEPIECE=ON
[03/23/2021 01:07:43] Using FBGEMM:
[03/23/2021 01:07:43] Unit tests:
[03/23/2021 01:07:43] Using CUDA visible devices:
[03/23/2021 01:07:43] Using number of GPU devices: 1
[03/23/2021 01:07:43] Using time out: 45m
[03/23/2021 01:07:43] Checking directory: speed-tests
[03/23/2021 01:07:44] Running speed-tests/test_wngt20_perf.sh ... OK
[03/23/2021 01:16:48] Test took 00:09:4.806s
---------------------
Ran 1 tests in 00:09:4.857s, 1 passed, 0 skipped, 0 failed

CMakeLists.txt

XapaJIaMnu · 2021-03-23T10:09:32Z

Can we also put the batch_translator inside the service? Nobody uses the batch translator by itself, as far as I can tell.

jerinphilip · 2021-03-23T10:19:45Z

I'm currently not in favour of this, as Service will move towards becoming a god class. With concurrency coming in, I'd have more and more functions packed. I want Service to handle the building and interactions (async or blocking) between constituent components (text-processsing, batching, translating, building-response).

Kenneth told me this can be functions instead, I haven't strictly understood how that happens either. My idea is store the graph and translation-related components (data), and translate a batch feature (function) there ensuring it's the unit which handles 'translation' and translation only. It's a functor in essence, which handles translation of a "Batch".

Maybe I am missing something, can you elaborate on how I can cleanly add it in Service without making Service something of a God class?

src/translator/CMakeLists.txt

jerinphilip · 2021-03-23T13:40:34Z

Regression tests with latest source, both seem passing.

jphilip@var:~/code/bergamot-translator-test-scripts$ MARIAN=/mnt/Storage/jphilip/bergamot-build/ ./run_brt.sh
[03/23/2021 13:03:51] Running on var as process 70614
[03/23/2021 13:03:51] Using Marian binary: /mnt/Storage/jphilip/bergamot-build/marian
[03/23/2021 13:03:51] Version: v1.9.56 370fdb5a 2021-03-10 17:12:37 +0000
[03/23/2021 13:03:51] Build type: Release
[03/23/2021 13:03:51] Using compiler: /usr/bin/c++
[03/23/2021 13:03:51] Using MKL: COMPILE_CPU=on
[03/23/2021 13:03:51] Using CUDNN:
[03/23/2021 13:03:51] Using SentencePiece: USE_SENTENCEPIECE=ON
[03/23/2021 13:03:51] Using FBGEMM:
[03/23/2021 13:03:51] Unit tests:
[03/23/2021 13:03:51] Using CUDA visible devices:
[03/23/2021 13:03:51] Using number of GPU devices: 1
[03/23/2021 13:03:51] Using time out: 5m
[03/23/2021 13:03:51] Checking directory: tests
[03/23/2021 13:03:51] Checking directory: tests/basic
[03/23/2021 13:03:52] Running tests/basic/test_bergamot_translator_app_intgemm_8bit.cpu-threads.0.sh ... OK
[03/23/2021 13:03:53] Test took 00:00:1.546s
[03/23/2021 13:03:53] Running tests/basic/test_service-cli_intgemm_8bit.cpu-threads.4.sh ... OK
[03/23/2021 13:03:56] Test took 00:00:3.029s
---------------------
Ran 2 tests in 00:00:4.667s, 2 passed, 0 skipped, 0 failed
jphilip@var:~/code/bergamot-translator-test-scripts$ MARIAN=/mnt/Storage/jphilip/bergamot-build/ TIMEOUT=45m BRT_THREADS=48 BRT_EXPECTED_MAXTIME=500 ./run_brt.sh speed-tests/test_wngt20_perf.sh

[03/23/2021 13:04:09] Running on var as process 70784
[03/23/2021 13:04:09] Using Marian binary: /mnt/Storage/jphilip/bergamot-build/marian
[03/23/2021 13:04:09] Version: v1.9.56 370fdb5a 2021-03-10 17:12:37 +0000
[03/23/2021 13:04:09] Build type: Release
[03/23/2021 13:04:09] Using compiler: /usr/bin/c++
[03/23/2021 13:04:09] Using MKL: COMPILE_CPU=on
[03/23/2021 13:04:09] Using CUDNN:
[03/23/2021 13:04:09] Using SentencePiece: USE_SENTENCEPIECE=ON
[03/23/2021 13:04:09] Using FBGEMM:
[03/23/2021 13:04:09] Unit tests:
[03/23/2021 13:04:09] Using CUDA visible devices:
[03/23/2021 13:04:09] Using number of GPU devices: 1
[03/23/2021 13:04:09] Using time out: 45m
[03/23/2021 13:04:09] Checking directory: speed-tests
[03/23/2021 13:04:09] Running speed-tests/test_wngt20_perf.sh ... OK
[03/23/2021 13:13:13] Test took 00:09:3.967s
---------------------
Ran 1 tests in 00:09:4.018s, 1 passed, 0 skipped, 0 failed

src/translator/batch_translator.h

abhi-agg

@jerinphilip The code looks fine to me. I left a comment regarding the usage of __EMSCRIPTEN__ pre-prcoessor 👍

kpu

If the only change is to the macro...

jerinphilip · 2021-03-23T15:14:51Z

I thank the creators of grep -R, cut, sort, uniq, xargs and sed -i. Can I get the final approval, now that builds are passing? Here are regression-tests passing for some cherry-on-top:

Regression tests: Pass

jphilip@var:~/code/bergamot-translator-test-scripts$ MARIAN=/mnt/Storage/jphilip/bergamot-build/ ./run_brt.sh
[03/23/2021 15:05:22] Running on var as process 74335
[03/23/2021 15:05:22] Using Marian binary: /mnt/Storage/jphilip/bergamot-build/marian
[03/23/2021 15:05:22] Version: v1.9.56 370fdb5a 2021-03-10 17:12:37 +0000
[03/23/2021 15:05:22] Build type: Release
[03/23/2021 15:05:22] Using compiler: /usr/bin/c++
[03/23/2021 15:05:22] Using MKL: COMPILE_CPU=on
[03/23/2021 15:05:22] Using CUDNN:
[03/23/2021 15:05:22] Using SentencePiece: USE_SENTENCEPIECE=ON
[03/23/2021 15:05:22] Using FBGEMM:
[03/23/2021 15:05:22] Unit tests:
[03/23/2021 15:05:22] Using CUDA visible devices:
[03/23/2021 15:05:22] Using number of GPU devices: 1
[03/23/2021 15:05:22] Using time out: 5m
[03/23/2021 15:05:22] Checking directory: tests
[03/23/2021 15:05:22] Checking directory: tests/basic
[03/23/2021 15:05:22] Running tests/basic/test_bergamot_translator_app_intgemm_8bit.cpu-threads.0.sh ... OK
[03/23/2021 15:05:23] Test took 00:00:1.514s
[03/23/2021 15:05:23] Running tests/basic/test_service-cli_intgemm_8bit.cpu-threads.4.sh ... OK
[03/23/2021 15:05:26] Test took 00:00:2.823s
---------------------
Ran 2 tests in 00:00:4.431s, 2 passed, 0 skipped, 0 failed
jphilip@var:~/code/bergamot-translator-test-scripts$ MARIAN=/mnt/Storage/jphilip/bergamot-build/ TIMEOUT=45m BRT_THREADS=48 BRT_EXPECTED_MAXTIME=500 ./run_brt.sh speed-tests/test_wngt20_perf.sh

[03/23/2021 15:05:28] Running on var as process 74488
[03/23/2021 15:05:28] Using Marian binary: /mnt/Storage/jphilip/bergamot-build/marian
[03/23/2021 15:05:28] Version: v1.9.56 370fdb5a 2021-03-10 17:12:37 +0000
[03/23/2021 15:05:28] Build type: Release
[03/23/2021 15:05:28] Using compiler: /usr/bin/c++
[03/23/2021 15:05:28] Using MKL: COMPILE_CPU=on
[03/23/2021 15:05:28] Using CUDNN:
[03/23/2021 15:05:28] Using SentencePiece: USE_SENTENCEPIECE=ON
[03/23/2021 15:05:28] Using FBGEMM:
[03/23/2021 15:05:28] Unit tests:
[03/23/2021 15:05:28] Using CUDA visible devices:
[03/23/2021 15:05:28] Using number of GPU devices: 1
[03/23/2021 15:05:28] Using time out: 45m
[03/23/2021 15:05:28] Checking directory: speed-tests
[03/23/2021 15:05:28] Running speed-tests/test_wngt20_perf.sh ... OK
[03/23/2021 15:14:15] Test took 00:08:47.155s
---------------------
Ran 1 tests in 00:08:47.206s, 1 passed, 0 skipped, 0 failed

Jerin Philip added 9 commits March 22, 2021 15:15

Merging two Services

eba8db9

Moving stop() logic to destructor

34a208a

We have WITH_PTHREADS back

dbdd581

string based constructor on Service

f92f3ed

Removing now empty service_base.* files

63ed071

Hiding away pcqueue_ construction

2c0f50c

Ugliest ifdefs I have done in my life.

Another ifdef to hide pcqueue header file

695510a

Missing semicolons in WITH_PTHREADS path

8e58688

Fixing async_translate residue argument from copy

6606c18

jerinphilip requested review from kpu, XapaJIaMnu and abhi-agg March 22, 2021 18:46

jerinphilip added the high-priority Needs to be done at the earliest label Mar 22, 2021

Jerin Philip added 2 commits March 22, 2021 18:50

Adding comments

8c9643c

Initialize batchtranslator only at one place

d474c4c

To reduce tax for bytebuffer loads, initialize batchtranslator only at one place.

kpu requested changes Mar 22, 2021

View reviewed changes

Jerin Philip added 6 commits March 22, 2021 23:23

\#ifdef WITH_PTHREADS -> #ifndef WASM_HIDE_THREADS

092b685

Sane platform (non WASM) is default. This truly only hide-threads from compilation path and not switch unswitch pthreads (-lpthread).

Review comments: Rearranging destructor, fix wrong comment

f2cde4f

Move loadVocabularies to service.cpp and put in anonymous namespace

6acaaed

Prettifying diff: Removing unwanted empty lines

ef1642c

Indicate in comments multithreaded has numWorkers translators

243b34d

Typo fix: bergamot_translator -> bergamot-translator

1eba44b

jerinphilip commented Mar 23, 2021

View reviewed changes

src/translator/service.cpp Outdated Show resolved Hide resolved

Jerin Philip added 2 commits March 23, 2021 00:46

Safety guards to avoid pcqueue illegal init

fe689b0

Add WASM_HIDE_THREADS as a global WASM_COMPILE_FLAG

dbb60f3

XapaJIaMnu reviewed Mar 23, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Compile Defs: WASM_HIDE_THREADS -> __EMSCRIPTEN__

7982d27

jerinphilip commented Mar 23, 2021

View reviewed changes

src/translator/CMakeLists.txt Outdated Show resolved Hide resolved

Removing dead CMakeLists.txt code following __EMSCRIPTEN__

8c89581

kpu previously approved these changes Mar 23, 2021

View reviewed changes

abhi-agg reviewed Mar 23, 2021

View reviewed changes

src/translator/batch_translator.h Outdated Show resolved Hide resolved

abhi-agg suggested changes Mar 23, 2021

View reviewed changes

Compile defs: __EMSCRIPTEN__ -> WASM

258f23e

jerinphilip dismissed kpu’s stale review via 258f23e March 23, 2021 14:44

kpu approved these changes Mar 23, 2021

View reviewed changes

jerinphilip linked an issue Mar 23, 2021 that may be closed by this pull request

Collapse Service to one class #61

Closed

abhi-agg self-requested a review March 23, 2021 16:32

abhi-agg approved these changes Mar 23, 2021

View reviewed changes

kpu merged commit 34228d3 into main Mar 23, 2021

kpu deleted the collapse-service branch March 23, 2021 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collapse Service #62

Collapse Service #62

jerinphilip commented Mar 22, 2021 •

edited

Loading

jerinphilip commented Mar 23, 2021 •

edited

Loading

XapaJIaMnu commented Mar 23, 2021

jerinphilip commented Mar 23, 2021 •

edited

Loading

jerinphilip commented Mar 23, 2021

abhi-agg left a comment

kpu left a comment

jerinphilip commented Mar 23, 2021 •

edited

Loading

Collapse Service #62

Collapse Service #62

Conversation

jerinphilip commented Mar 22, 2021 • edited Loading

jerinphilip commented Mar 23, 2021 • edited Loading

XapaJIaMnu commented Mar 23, 2021

jerinphilip commented Mar 23, 2021 • edited Loading

jerinphilip commented Mar 23, 2021

abhi-agg left a comment

Choose a reason for hiding this comment

kpu left a comment

Choose a reason for hiding this comment

jerinphilip commented Mar 23, 2021 • edited Loading

jerinphilip commented Mar 22, 2021 •

edited

Loading

jerinphilip commented Mar 23, 2021 •

edited

Loading

jerinphilip commented Mar 23, 2021 •

edited

Loading

jerinphilip commented Mar 23, 2021 •

edited

Loading