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

[Change] 不再兼容旧格式示例代码 #56573

Merged
merged 10 commits into from
Aug 25, 2023

Conversation

megemini
Copy link
Contributor

@megemini megemini commented Aug 23, 2023

PR types

Others

PR changes

Others

Description

中国软件开源创新大赛:飞桨框架任务挑战赛
赛题五:将 xdoctest 引入到飞桨框架工作流中

RFC [Add]将 xdoctest 引入到飞桨框架工作流中v1
ISSUE 赛题五:将 xdoctest 引入到飞桨框架工作流中 Tracking Issue

第二阶段
任务:不再兼容旧格式

这里没有直接删除旧格式的检查。

如果 commit 中包含 test=document_legacy,则开启旧格式的检查,以保持兼容性,或可以比对之前的检查结果(典型的如 distributed 目录)。

此参数默认不开启,即,默认不支持旧格式,如果检查出来,则直接报错并退出。

涉及文件:

  • paddle/scripts/paddle_build.sh 用于检查 commit 中 test=document_legacy 关键字。

  • tools/sampcd_processor.py 如果不使用 legacy,并检查出示例代码,则出错。

  • tools/sampcd_processor_utils.py 添加 legacy 关键字。

  • python/paddle/tensor/creation.py 临时文件,用于检查修改的正确性。

    • 这里先没有使用 test=document_legacy,预期 CI 会报错并退出。
    • 后面会修改 commit,检查 test=document_legacy 关键字可用。

直接删除掉原有 sampcd_processor.pysampcd_processor_xdoctest.py 代替。

替换后,检查为 failed, timeout, nocode(无新样式示例代码或语法错误) 则提示错误并退出。

涉及文件:

  • sampcd_processor.py 删除
  • sampcd_processor_xdoctest.py 更名为 sampcd_processor.py
  • test_sampcd_processor.py 删除
  • test_sampcd_processor_xdoctest.py 更名为 test_sampcd_processor.py
  • python/paddle/tensor/creation.py 临时文件,用于检查修改的正确性。预期为:全部 nocode

@SigureMo @luotao1 @jzhang533 @sunzhongkai588

请评审,谢谢!:)

@luotao1
Copy link
Contributor

luotao1 commented Aug 23, 2023

如果旧格式没有了,旧检查也删除吧,以后也没有人会维护了呢

@SigureMo
Copy link
Member

如果旧格式没有了,旧检查也删除吧,以后也没有人会维护了呢

嗯,中文下的也可以一并删除了

@megemini
Copy link
Contributor Author

嗯,中文下的也可以一并删除了

这个是 docs 下面的检查是吧? 我在 docs 那边另起 PR 哈 ~

@paddle-bot paddle-bot bot added the contributor External developers label Aug 23, 2023
@SigureMo
Copy link
Member

image

我们是不是应该检测到旧格式时进行提醒,并指引参考 https://www.paddlepaddle.org.cn/documentation/docs/zh/develop/dev_guides/style_guide_and_references/code_example_writing_specification_cn.html ,而不是新旧格式一并使用 xdoctest 直接检查?

Comment on lines 16 to 21
usage: python sample_test.py {cpu or gpu}
usage: python sampcd_processor_xdoctest.py {cpu or gpu}
{cpu or gpu}: running in cpu version or gpu version

for example, you can run cpu version testing like this:

python sampcd_processor.py cpu
Copy link
Member

Choose a reason for hiding this comment

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

这俩地方名字是不是不用改了?

@megemini
Copy link
Contributor Author

megemini commented Aug 24, 2023

我们是不是应该检测到旧格式时进行提醒,并指引参考 https://www.paddlepaddle.org.cn/documentation/docs/zh/develop/dev_guides/style_guide_and_references/code_example_writing_specification_cn.html ,而不是新旧格式一并使用 xdoctest 直接检查?

呃 ... ... 我之前就是这么做的,可是你们说干脆删掉就行了 ... ...

而且,最麻烦的地方是,旧格式其实没有一个检查标准,对于 xdoctest 来说,旧格式就是在 .. code-block:: python 中没有 >>> 提示的代码,说白了,旧格式的代码跟普通文本没什么区别 ~


这样吧,我在 sampcd_processor_utils.py 里面加个检查试试看 ~

@SigureMo
Copy link
Member

这样吧,我在 sampcd_processor_utils.py 里面加个检查试试看 ~

嗯,我觉得以前那种检查就挺好的(re.search(r"\n>>>\s?", "\n" + codes)),目前还没发现有误判的情况

文档的 copy button 当时给他们的 demo 也是用的这个逻辑(当然是改成 JS),毕竟 copy button 还是要同时兼容新旧格式的

@SigureMo
Copy link
Member

现在报错怎么不在最下面了呢

image

后面还有些其他信息,不太容易找到有效报错信息

Copy link
Member

Choose a reason for hiding this comment

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

arguments = [
    # flags, dest, type, default, help
    ['--gpu_id', 'gpu_id', int, 0, 'GPU device id to use [0]'],
    ['--logf', 'logf', str, None, 'file for logging'],
    ['--threads', 'threads', int, 0, 'sub processes number'],
]

这里这个直接展开写成

    parser.add_argument('--debug', dest='debug', action="store_true")
    parser.add_argument('--full-test', dest='full_test', action="store_true")
    parser.add_argument('mode', type=str, help='run on device', default='cpu')

的形式吧,docs repo 那边也一样,这种写法很奇怪,也没必要

@megemini
Copy link
Contributor Author

后面还有些其他信息,不太容易找到有效报错信息

可能是 logger 的问题 ~ 我改一下试试 ~

type=int,
default=0,
help='sub processes number',
)

if len(sys.argv) == 1:
Copy link
Member

Choose a reason for hiding this comment

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

这里的逻辑判断好奇怪,明明 required 还要设置默认值

我们修改一下吧,把 mode 改成 --mode,当然 CI 里的调用也需要改一下,这里的 if 也不需要了

Copy link
Member

Choose a reason for hiding this comment

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

看起来已经没什么问题了,相关测试修改要恢复么

Copy link
Member

@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.

LGTMeow 🐾

@luotao1 luotao1 merged commit 9208069 into PaddlePaddle:develop Aug 25, 2023
BeingGod pushed a commit to BeingGod/Paddle that referenced this pull request Sep 9, 2023
* [Change] forbid legacy sample code style

* [Change] remove legacy sampcd_processor.py

* [Fix] fix command

* [Change] check plain style sample code

* [Fix] fix logger

* [Change] remove arguments

* [Change] remove cb_required

* [Change] change parse args

* [Change] restore creation.py
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.

4 participants