PyThaiNLP / PyThaiNLP/pythainlp

bug: Seq2Seq teacher forcing disabled by unconditional overwrite — intentional?

Open
#1,390 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Python
Stars
1.2k
Forks
304
Avg merge
2d 14h
Merged PRs (30d)
14

Description

Description

In Seq2Seq.forward() (transliterate/thai2rom.py), the teacher forcing if-else block is immediately overwritten by an unconditional assignment:

# line 421-426
if teacher_force and target_seq is not None:
    decoder_input = target_seq[:, di].reshape(batch_size, 1)
else:
    decoder_input = topi.detach()

decoder_input = topi.detach()  # ← this always runs, overwriting the above

Line 426 makes the if-else block dead code — teacher_forcing_ratio has no effect.

History

Looking at git history, this appears to be an incomplete revert:

  1. fd4b6676 (2019-08-02) — Original: teacher forcing with ternary expression. Correct.
  2. dd6410d4 (2022-10-24) — Removed teacher forcing, left decoder_input = topi.detach().
  3. d48dc503 (2022-10-25) — "fix: bringback original thai2rom code" — added back the if-else but did not remove the old line from step 2.
Question

Was removing teacher forcing in step 2 intentional? If the intent of step 3 was to restore it, then line 426 should be removed. The pre-trained model shipped with PyThaiNLP was trained before this change, so romanize(engine="thai2rom") still works — only retraining/fine-tuning is affected.

If teacher forcing is no longer needed (e.g. the model is only used for inference), the if-else block at lines 421-424 could be removed instead.

Steps to reproduce
import inspect
from pythainlp.transliterate.thai2rom import Seq2Seq

source = inspect.getsource(Seq2Seq.forward)
# Lines 421-426 show the if-else followed by unconditional overwrite
PyThaiNLP version

5.3.3

Files
  • pythainlp/transliterate/thai2rom.py (lines 421-426)

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in pythainlp/transliterate/thai2rom.py at Seq2Seq.forward() and inspect the teacher-forcing block around lines 421-426. Compare commits fd4b6676, dd6410d4, and d48dc503, then use the inspect-based reproduction to confirm the current behavior. Done means the intended training behavior is established and the unconditional overwrite or dead block is resolved accordingly.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
machine-learning
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.