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

[CodeStyle][py2] remove six package (part2) #47334

Merged
merged 15 commits into from
Nov 1, 2022

Conversation

SigureMo
Copy link
Member

@SigureMo SigureMo commented Oct 25, 2022

PR types

Others

PR changes

Others

Describe

清理 six 模块的第二步(同时也是最后一步)

  • six.reraise(不修改行为,只是根据情况调整以获得更好的报错信息)
    • except 块里无其他操作或者为 StopIteration -> 直接 raise
    • except 块里有其他操作 -> raise e
    • 冗余的 try-except(except 里直接 reraise,无其余操作,如 python/paddle/fluid/executor.py),直接移除
  • six.ensure_str(类似 paddle.compat.to_text,根据情况修改)
    • 根据 etcd 文档和源码及运行时实测,当前所使用的 six.ensure_str 的参数(即 Etcd3Client.getEtcd3Client.get_prefix 的返回值) 全都需要 decode,单测里 Mock 的数据是有问题的
  • dy2static convert_call_func BUILTIN_LIKELY_MODULES 列表中的 six
    • 影响:只是动转静过程中不将 six 下的函数直接视为 builtin 函数,而由于 six 下的函数大多会调用 builtin 函数,被调用的函数还是 builtin 的,实际不影响用户在动转静函数里使用 six 下的函数(如 six.iteritems
  • 两个示例代码中的 six 使用
  • six 依赖项,含 conda 和 pip(pypi)的依赖

全局搜索 import six / from six 已无残余,搜索 six 也没有相关残余,应当本 PR 即可完全清理

Related links

@paddle-bot
Copy link

paddle-bot bot commented Oct 25, 2022

你的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 contributor External developers status: proposed labels Oct 25, 2022
core.update_autotune_status()
return res
except Exception as e:
six.reraise(*sys.exc_info())
Copy link
Member Author

@SigureMo SigureMo Oct 25, 2022

Choose a reason for hiding this comment

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

这里于 #18482#18957 在 reraise 之前有一个针对 core.EOFException 的处理,该处理已于 #25692 移除,但没有清理外面的 try-except 结构,因此这里直接清除掉

@luotao1 luotao1 self-assigned this Oct 26, 2022
@SigureMo SigureMo changed the title [WIP][CodeStyle][py2] remove six package (part2) [CodeStyle][py2] remove six package (part2) Oct 27, 2022
@@ -27,6 +27,16 @@ def refresh(self):
pass


class MockKVMetadata:
Copy link
Contributor

Choose a reason for hiding this comment

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

可以写一下这个单测修改的思路

Copy link
Member Author

Choose a reason for hiding this comment

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

嗯嗯,刚刚过完 CI 正准备写一下 😂

Copy link
Member Author

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

上次 PR 修改 API docs 由于直接修改会出问题而回撤的两个,需要额外修改一下才能通过 CI


def print_prog(prog):
for name, value in sorted(six.iteritems(prog.block(0).vars)):
for name, value in sorted(prog.block(0).vars.items()):
Copy link
Member Author

Choose a reason for hiding this comment

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

API docs changes 1:

这里前面不加一个 import 的话从 docstring 提取出来的源码格式会有问题导致无法运行,暂时加了一个 import paddle

不过这个问题本地直接用 sampcd_processor.py 提取出来的是正常的,格式不会出问题,不太清楚 CI 上为啥会出问题,暂时没有仔细研究

res = simple_net(x, y)

exe = paddle.static.Executor(paddle.CPUPlace())
exe.run(paddle.static.default_startup_program())
input1 = np.random.random(size=[1,4]).astype('float32')
input2 = np.random.randint(1, 10, size=[1,10], dtype='int64')
input2 = np.random.randint(1, 10, size=[1], dtype='int64')
out = exe.run(paddle.static.default_main_program(),
Copy link
Member Author

Choose a reason for hiding this comment

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

API docs changes 2:

CrossEntropyLossy shape 为 [1] 即可

Copy link
Member Author

@SigureMo SigureMo left a comment

Choose a reason for hiding this comment

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

关于 six.ensure_str 的修改以及相关 etcd 单测修改的说明


def cancel_watch(self, watch_id):
pass

def delete(self, key):
pass
return True
Copy link
Member Author

@SigureMo SigureMo Oct 27, 2022

Choose a reason for hiding this comment

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

这里是用于 mock etcd3.Etcd3Client 行为的类,但实际上很多返回数据和真实的 etcd3.Etcd3Client 方法返回数据是不一致的,比如 get 方法实际上返回 tuple[bytes, KVMetadata]get_prefix 返回 Iterable[tuple[bytes, KVMetadata]],与原来 MockEtcdClient 这两个方法返回数据类型是完全不一致的,这导致了将 six.ensure_str(x) 修改为 x.decode() 单测会报错

因此这里根据 etcd3 文档、源码以及运行时实测的具体数据类型,修改了这里 mock 的数据,其中 KVMetaData 用 mock 的 MockKVMetadata 代替

Copy link
Member Author

@SigureMo SigureMo Oct 27, 2022

Choose a reason for hiding this comment

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

由于源码里 six.ensure_str 都是对 getget_prefix 返回结果进行操作的,是确定的 bytes 类型,因此相关位置全部修改为了 decode

Copy link
Contributor

Choose a reason for hiding this comment

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

@xymyeah review下[test_fleet_elastic_manager.py]的修改

@luotao1 luotao1 requested a review from xymyeah October 28, 2022 02:39
@@ -35,28 +45,30 @@ def put(self, key, value, lease=None):
pass

def get(self, key):
value = "0"
return value, value
return b'0', MockKVMetadata(b"/prefix")
Copy link
Contributor

Choose a reason for hiding this comment

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

之前也是基于etcd3的mock,不修改会有问题是吗?不确定这样修改后是否可以正常运行单测

Copy link
Member Author

Choose a reason for hiding this comment

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

对的,mock 的数据返回的是 str,而实际应该是 bytes,在原来的代码里使用 six.ensure_str(x) 可以保证这两种都没问题,但如果需要移除 six 的话就需要根据情况判断了,这里我认为返回数据是确定的 bytes 就直接使用 x.decode() 了,但会因为 mock 数据是 str 而出问题

如果修改的风险不可接受,我觉得单独定义一个 _ensure_str 来模拟原来 six.ensure_str 行为,可以保证对代码没有任何影响

Copy link
Contributor

Choose a reason for hiding this comment

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

这个单测主要是用来mock etcd3,修改后不产生影响就行,其他没啥问题

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@xymyeah xymyeah left a comment

Choose a reason for hiding this comment

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

LGTM

@luotao1 luotao1 merged commit 3592ba8 into PaddlePaddle:develop Nov 1, 2022
@SigureMo SigureMo deleted the py2/fix/remove-six-part-2 branch November 1, 2022 07:12
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.

7 participants