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

Support fetch in new ir #54826

Merged
merged 6 commits into from Jun 25, 2023
Merged

Conversation

phlrain
Copy link
Collaborator

@phlrain phlrain commented Jun 23, 2023

PR types

Others

PR changes

Others

Description

在新IR体系下,支持fetch 变量

Other

Pcard-67164

@paddle-bot
Copy link

paddle-bot bot commented Jun 23, 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
Copy link

paddle-bot bot commented Jun 23, 2023

❌ The PR is not created using PR's template. You can refer to this Demo.
Please use PR's template, it helps save our maintainers' time so that more developers get helped.

@@ -17,8 +17,20 @@
data_transform: {}
attrs:
- {typename: str, name: name}
outputs: []
outputs:
- {typename: Tensor, name: out, optional: false, intermediate: false}
Copy link
Contributor

Choose a reason for hiding this comment

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

这里目前的yaml语义是暂时只支持一个Tensor么?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

是的,fetch的时候,一个fetch op 拿一个数据,有多个tensor的时候,会有多个fetch op


ctx->EmplaceBackOutput(scope->Var(name)->Get<phi::DenseTensor>());
// update here, support fetch list for now
// [todo update here]
Copy link
Contributor

Choose a reason for hiding this comment

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

后续也可以注意下框架里的「注释规范」,对于待办项,应该是

// TODO(author): xxxx   <--- 表述文案应遵循应为语法、和报错规范一致

(op->attributes().at("op_name").dyn_cast<ir::StrAttribute>().data() ==
"pd.fetch")) {
// process fetch op
auto fetch_var = scope->Var("fetch");
Copy link
Contributor

Choose a reason for hiding this comment

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

疑问:新PR里允许存在多个 pd.fetch 算子么?若允许存在,那多个pd.fetch的输出var的name在scope里都是"fetch" 么?有重名的风险么?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这当前的逻辑,由于历史原因,非常的trick, fetch的Var("fetch"对应的var) 是一个fetchlist,里面包含很多个tensor,每个op处理这fetchList里面的一个元素,所以不会有重名的风险

由于这里涉及到executor python端,很大一片的修改,可能还有兼容性的问题,暂时没有做修改

if (output_map != nullptr) {
// only deal with single input for now, [todo] need support multi input
// like concat
size_t tmp_id = std::atol(name.substr(4, 100).c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

substr(4, 100)这里的 4 和 100 两个magic number 的含义是什么?上下文没有相关的comment注释

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这我备注下, 这里是给新执行里面的OpFuncNode中的 inputs,和outputs设置正确的id, 来保证执行的正确性

@@ -952,10 +952,11 @@ void BuildOpFuncList(

auto op_name = attr_map.at("op_name").dyn_cast<::ir::StrAttribute>().data();

if (op_name == "pd.fetch" || op_name == "builtin.combine") {
if (op_name == "builtin.combine") {
VLOG(6) << "skip process pd.fetch op";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
VLOG(6) << "skip process pd.fetch op";
VLOG(6) << "skip process " << op_name;

Copy link
Contributor

@winter-wang winter-wang left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@qili93 qili93 left a comment

Choose a reason for hiding this comment

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

LGTM for const_cast

@phlrain phlrain merged commit e66beb0 into PaddlePaddle:develop Jun 25, 2023
24 of 25 checks passed
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.

None yet

6 participants