issues with classify_review method

[Jessen-Li]

In page 201,

def classify_review(
text, model, tokenizer, device, max_length=None,
pad_token_id=50256):
  model.eval()
  
  input_ids = tokenizer.encode(text)
  supported_context_length = model.pos_emb.weight.shape[1]
  input_ids = input_ids[:min(
        max_length, supported_context_length
  )]
  input_ids += [pad_token_id] * (max_length - len(input_ids))

issue 1: model.pos_emb.weight.shape is [1024,768], of which 1024 is the context length and 768 is the dimension of the embeddings. So, should it be shape[0] instead?

issue 2: if supported_contex_length is smaller than max_length, it makes sense to truncate the input_ids to supported_contex_length. But in this case, what's the meaning of expand to max_length? Though in latter code, calling of classify_review uses max_length of 120 (train_dataset.max_length), which is much smaller than supported_context_length.
So a better way to handle both cases is to:

     supported_context_length = model.pos_emb.weight.shape[0]
    max_len = min(max_length,supported_context_length) if max_length else supported_context_length
    input_ids = input_ids[:max_len]
    input_ids += [pad_token_id] * (max_len - len(input_ids)) 

It also takes care of the case when the max_length is None.

[casinca]

Hello,
Concerning your issue1, you're right it was a typo in the book I think I remember someone already bringing that up, in the notebook, Sebastian fixed and wrote in comment:

# Note: In the book, this was originally written as pos_emb.weight.shape[1] by mistake
# It didn't break the code but would have caused unnecessary truncation (to 768 instead of 1024)

For issue2, I also did it like you, reassigning max_len so I didn't have that problem, but I think you're right that it is a good catch.
With default arg max_len=None, it will return a TypeError. It never happens in the book/notebook because Sebastian reassigns that argument every time.

[Jessen-Li]

should I submit an issue?

[casinca]

should I submit an issue?

Yeah I think it's justified. An issue is more appropriate than a PR here, because since it's both in the book and the repo, I'm not sure how Sebastian wants to handle it. I know he wants as little deviation as possible repo↔book.

[d-kleine]

For reference, this was the original discussion: #499