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

upgrade torch.median and 3 apis #382

Merged

Conversation

RedContritio
Copy link
Contributor

Copy link

paddle-bot bot commented Apr 2, 2024

Thanks for your contribution!

@paddle-bot paddle-bot bot added the contributor External developers label Apr 2, 2024
Copy link
Collaborator

@zhwesky2010 zhwesky2010 left a comment

Choose a reason for hiding this comment

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

这一次PaConvert升级,尽可能要彻底解决某些API的遗留问题

@@ -4328,3 +4328,46 @@ def generate_code(self, kwargs):
else:
code = "misidentify"
return code


class MedianMatcher(BaseMatcher):
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里目前的API差异还有在哪里?如果还有这么大的差异就不太符合 API升级 这个项目的意义了。

预期的API修改效果是 mode='min' 之后完全对齐,如果还有对不齐的地方可以反馈给 负责API升级的同学 来返工修复,直到满足要求为止。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个差异的原因是,median 有两个重载,如果只有 input 参数,则只返回值;如果有其他参数,则返回值和 indice。
所以需要针对两种重载来处理 out 参数(赋值单个或者两个)

Copy link
Collaborator

@zhwesky2010 zhwesky2010 Apr 3, 2024

Choose a reason for hiding this comment

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

  • 如果没有out,就视作完全一致
  • 如果有out,就必然有dim,也就是返回值和indice,因此赋值两个

只需要这样写就可以了,只有这两种情况,不用写得那么复杂。

整个reduce系列都与这种类似(torch.min还额外有一种torch.minimum的用法与之不同),可以在TupleAssignMatcher的基础上优化一个通用Matcher。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

)


def test_case_5():
# 会引发段错误,先屏蔽
def _test_case_5():
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里的Bug反馈过去了吗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已经反馈

@@ -8495,7 +8495,7 @@
}
},
"torch.median": {
"Matcher": "GenericMatcher",
"Matcher": "MedianMatcher",
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里是不是直接用doubleassignmatcher就可以了

@@ -8715,7 +8715,7 @@
}
},
"torch.nanmedian": {
"Matcher": "GenericMatcher",
"Matcher": "MedianMatcher",
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里是不是直接用doubleassignmatcher就可以了

)


def test_case_3():
# 频繁测试会引发段错误,先屏蔽
def _test_case_3():
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个问题需要nanmedian看下

Copy link
Collaborator

@zhwesky2010 zhwesky2010 left a comment

Choose a reason for hiding this comment

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

目前nanmedian已经修复了,CI应该可以通过了

@@ -3336,6 +3342,12 @@ def generate_code(self, kwargs):
else:
kwargs.pop(k)

paddle_default_kwargs = self.api_mapping.get("paddle_default_kwargs", {})
Copy link
Collaborator

@zhwesky2010 zhwesky2010 Apr 8, 2024

Choose a reason for hiding this comment

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

基类已经封装了设置默认参数的功能,TripleAssignMatcher 已经用了,见代码: https://github.com/PaddlePaddle/PaConvert/blob/master/paconvert/api_matcher.py#L3327

DoubleAssignMatcher 也换成这种封装好的写法

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已更新

@zhwesky2010
Copy link
Collaborator

@RedContritio TransformerDecoderLayer 这个API也支持了layernorm_eps

Copy link
Collaborator

@zhwesky2010 zhwesky2010 left a comment

Choose a reason for hiding this comment

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

TransformDecodeLayer这个单测没有改,也移除了layer_norm_eps,需要把单测升级一下

pytorch_code,
["result"],
unsupport=True,
reason="paddle unsupport layer_norm_eps args",
Copy link
Collaborator

Choose a reason for hiding this comment

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

TransformDecodeLayer这个的单测是不是没改?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,下个 pr 改

@zhwesky2010 zhwesky2010 merged commit b6cda71 into PaddlePaddle:master Apr 10, 2024
8 checks passed
@RedContritio RedContritio deleted the update_median_etc_matcher branch April 10, 2024 08:37
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.

2 participants