Skip to content
This repository was archived by the owner on Apr 29, 2021. It is now read-only.

RNN-T#7

Open
julianmack wants to merge 253 commits into
masterfrom
rnnt
Open

RNN-T#7
julianmack wants to merge 253 commits into
masterfrom
rnnt

Conversation

@julianmack

@julianmack julianmack commented Nov 11, 2019

Copy link
Copy Markdown
Contributor

Adding Transducer (including RNNT model, transducer_loss and transducer_decoders). Also added CER calculation.

Note that, in order to organise the speech_to_text builder by loss it was necessary to alter the code order (to consider loss before model) meaning that the following files appear to have many more changes than they actually do:

  • src/myrtlespeech/builders/speech_to_text.py
  • tests/protos/test_speech_to_text.py

Comment thread docs/source/install.rst

@julianmack julianmack left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not ready for review
edit: out of date. PR now ready

@julianmack julianmack added the not ready WIP pull request not ready to be merged yet label Nov 12, 2019
@julianmack julianmack force-pushed the rnnt branch 2 times, most recently from ec3838d to 4610468 Compare November 13, 2019 14:47
Comment thread tests/builders/test_speech_to_text.py Outdated
Comment on lines +21 to +32
try:
stt: Union[SpeechToText, None] = None
stt = build(stt_cfg)
except AttributeError:
warnings.warn(
"This test has been (partially) disabled. TODO: remove this \
exception catching."
)
if stt is not None:
assert isinstance(stt, SpeechToText)
assert isinstance(stt, SeqToSeq)
warnings.warn("SpeechToText only built and not checked if correct")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@julianmack - remove this hack? This prevents failing tests when st draws a crossover ctc/rnnt loss and network

Comment thread tests/builders/test_task_config.py Outdated
Comment on lines +19 to +37
try:
model, epochs, train_loader, eval_loader = build(task_cfg)
if model is not None:
assert isinstance(model, torch.nn.Module)
else:
warnings.warn(
"Not checking if model is returned. Remove above `if` \
statement once tests/protos/test_speech_to_text.py has had exception handling removed"
)

assert isinstance(epochs, int)
assert isinstance(train_loader, torch.utils.data.DataLoader)
assert isinstance(eval_loader, torch.utils.data.DataLoader)
warnings.warn("TaskConfig only built and not checked if correct")
except ValueError as e:
if str(e) == "unsupported model None":
warnings.warn(f"Caught error {e}.")
else:
raise e

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@julianmack - remove this hack? This prevents failing tests when st draws a crossover ctc/rnnt loss and network

@julianmack julianmack left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Still to do:

  • Refactor decoder
  • Add a few tests (rnn w. hidden state and rnn_t loss in particular)
  • Remove hacks that prevent stt test failing when there is an rnnt network and ctc loss.

@julianmack julianmack removed the not ready WIP pull request not ready to be merged yet label Nov 13, 2019
@julianmack julianmack requested a review from samgd November 13, 2019 15:55
@julianmack julianmack force-pushed the rnnt branch 2 times, most recently from b6de02d to b5bb579 Compare November 21, 2019 17:34
Comment thread Dockerfile Outdated
Comment thread tests/builders/test_rnn_t.py Outdated
@julianmack julianmack marked this pull request as ready for review November 25, 2019 15:06

@samgd samgd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great to have this working! The largest review theme is to generalise the transduction loss/decoding from being specific to an RNN. Yet to review the builders or tests under the assumption that they will change significantly when updating based on the comments.

Comment thread .gitignore Outdated
Comment thread Dockerfile
Comment thread Dockerfile Outdated
Comment thread Dockerfile Outdated
Comment thread Dockerfile Outdated
Comment thread src/myrtlespeech/run/run.py Outdated
Comment thread src/myrtlespeech/run/run.py Outdated
Comment thread src/myrtlespeech/run/run.py
Comment thread src/myrtlespeech/run/train.py
Comment thread src/myrtlespeech/run/callbacks/callback.py Outdated
@julianmack julianmack requested a review from samgd November 28, 2019 14:13

@samgd samgd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yet to review the docs, tests, and builders under the assumption that they may change significantly based on comments.

Comment thread Dockerfile
Comment on lines 41 to +43
cd .. && \
rm -rf apex

# install warp-transducer
ENV CXX=/usr/bin/g++-6
ENV CC=/usr/bin/gcc-6
RUN make deps/warp-transducer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can both warp-transducer and apex be installed via the Makefile so the Makefile becomes the single point of call for dependency installation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It can but in the docs it states that:

The Dockerfile installs NVIDIA Apex, used for mixed precision, using a Python-only build and will omit some Apex features and performance improvements.

and I assumed that this was because CI doesn't like the proper apex build.

This might have been the wrong assumption - should we be using the cpp build for CI?

Comment thread Dockerfile Outdated
Comment thread Makefile Outdated
Comment thread pytest.ini Outdated
Comment thread src/myrtlespeech/model/transducer.py Outdated
Comment thread src/myrtlespeech/protos/transducer_beam_decoder.proto Outdated
Comment thread src/myrtlespeech/run/callbacks/rnn_t_training.py Outdated
Comment thread src/myrtlespeech/run/eval.py Outdated
Comment thread src/myrtlespeech/run/train.py
Comment thread src/myrtlespeech/run/run.py Outdated
Comment thread environment.yml
- pip=19.2.2=py37_0
- pluggy=0.12.0=py_0
- pre_commit=1.17.0=py37_0
- pre_commit=1.11.2=0

@julianmack julianmack Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have worked out what the pre-commit problem was. The pre-commit update from version 1.11.2 -> 1.12 broke the reorder_python_imports integration.
So I've conda installed a more up-to-date version (which updates a few other core packages hence the other edits here)

Comment thread Dockerfile Outdated
Comment thread src/myrtlespeech/model/transducer.py
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants