Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[phi] fix sequence_mask dtype #60423

Merged
merged 12 commits into from
Jan 17, 2024
Merged

Conversation

gouzil
Copy link
Member

@gouzil gouzil commented Dec 28, 2023

PR types

Others

PR changes

Others

Description

去除 sequence_mask 中不必要的类型转换

问题如下:

在pir模式下也会调用TransToPhiDataType导致原本正常的DataType又当成ProtoDataType再次转换,导致类型不正确

TODO:

这里不删是因为,这是测 PT 的,我们开 check_pir 是测端到端理想态的,还不一样

clean codetest/sequence/CMakeLists.txt

- set(PIR_COVERAGE_TESTS test_sequence_mask)

- foreach(PIR_COVERAGE_TEST ${PIR_COVERAGE_TESTS})
-  py_test_modules(${PIR_COVERAGE_TEST}_pir MODULES ${PIR_COVERAGE_TEST} ENVS
-                  FLAGS_enable_pir_in_executor=true)
-  set_tests_properties(${PIR_COVERAGE_TEST}_pir PROPERTIES TIMEOUT 120)
-  message(STATUS "PIR Copied OpTest: ${PIR_COVERAGE_TEST}_pir in sequence test")
- endforeach()

Copy link

paddle-bot bot commented Dec 28, 2023

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot paddle-bot bot added the contributor External developers label Dec 28, 2023
@gouzil gouzil changed the title [Don't merge] test sequence_mask [phi] fix sequence_mask dtype Jan 4, 2024
Comment on lines 110 to 111
if in_pir_mode() and isinstance(dtype, core.VarDesc.VarType):
dtype = vartype_to_datatype[dtype]
Copy link
Member

Choose a reason for hiding this comment

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

与震哥对齐,这个 API 最好不要加这个兼容逻辑,已经发现 OpTest 存在的问题,问题先记录了,这个 PR 暂时 delay 处理

Copy link
Member

Choose a reason for hiding this comment

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

依赖已解,继续推进本 PR

Copy link

paddle-ci-bot bot commented Jan 12, 2024

Sorry to inform you that f81740a's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.

@@ -235,11 +235,6 @@ def __setattr__(self, name, val):
}


# FIXME(dev): We haven't fully verified eager mode on XPU et.al but
# only GPU/CPU. Remove this after we improve this feature.
_is_first_import_ = True
Copy link
Member

Choose a reason for hiding this comment

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

顺带清理了下这里,这个是 #41774 引入的,其他代码都已经清理了,但是这个全局变量没清

Copy link
Contributor

@YuanRisheng YuanRisheng left a comment

Choose a reason for hiding this comment

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

这里可能需要确认一下,修改的这几个kernel,npu等设备kernel也是否用到了,如果用到了的话,那些kernel的参数也需要同步修改,npu等设备Kernel在PaddleCustomDevice代码仓库中

Copy link
Contributor

@Aurelius84 Aurelius84 left a comment

Choose a reason for hiding this comment

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

LGTM

@SigureMo
Copy link
Member

这里可能需要确认一下,修改的这几个kernel,npu等设备kernel也是否用到了,如果用到了的话,那些kernel的参数也需要同步修改,npu等设备Kernel在PaddleCustomDevice代码仓库中

我看了下,NPU 这里已经是 DataType,是不是不需要修改呢?

https://github.com/PaddlePaddle/PaddleCustomDevice/blob/b624900f33ee25c4bf3d98dc2044abdf5d6934cb/backends/npu/kernels/sequence_mask.cc#L21-L27

@SigureMo SigureMo merged commit f021ddd into PaddlePaddle:develop Jan 17, 2024
@gouzil gouzil deleted the test_ci_sequence_mask branch April 23, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor External developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants