Skip to content

Conversation

@Yam0214
Copy link
Contributor

@Yam0214 Yam0214 commented Nov 3, 2022

PR types

New features

PR changes

Models

Description

  1. Add embedding inputs to T5 model
  2. Add test_inputs_embeds to test_modeling_common.py

test_resize_position_embeddings = False
test_mismatched_shapes = True
test_missing_keys = True
use_test_inputs_embeds = False
Copy link
Contributor

Choose a reason for hiding this comment

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

这里的 use_test_inputs_embeds 好像在下面的单测中没有用到了?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已补上

inputs["decoder_inputs_embeds"] = wte(decoder_input_ids)

with paddle.no_grad():
model(**inputs)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

这里的测试逻辑是要求他不报错吗?能否加上一个直接用input_ids跑和input_embeds跑的assertEquals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

接受提议并修改完成

with paddle.no_grad():
embeds_output = model(**inputs)

assert paddle.allclose(ids_output, embeds_output, 1e-4, 1e-4)
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 这里不能直接用python的assert. 需要用unittest这个class的assert
  2. 代码风格上,最好把1e-4的的key加上
self.assertTrue(paddle.allclose(ids_output, embeds_output, atol=1e-4))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已修改

@PaddlePaddle PaddlePaddle locked and limited conversation to collaborators Nov 11, 2022
@PaddlePaddle PaddlePaddle unlocked this conversation Nov 11, 2022
@FrostML FrostML merged commit 3ca3547 into PaddlePaddle:develop Nov 14, 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.

3 participants