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

[Add]将 xdoctest 引入到飞桨框架工作流中v1 #547

Merged
merged 6 commits into from
Jun 15, 2023

Conversation

megemini
Copy link
Contributor

PR types

New features

PR changes

Docs

Describe

[used AI Studio]

中国软件开源创新大赛:飞桨框架任务挑战赛

之前的 PR #540 调研不全面,我重新写了这一版,后面把那个 PR 关了吧~

另外,补充几个问题:

  • 后续是否可以移除 Paddle docs 中的代码检查?这影响,是否后续需要同时维护两块代码。
  • Paddle docs 的 sphinx 文档编译还有不清楚的地方,PR 里面也写了,这里可否指导一下~
  • 示例代码的量太大(近2000)段,有没有好的修改方法?或者搞个快乐开源让大家都来玩玩?!

@SigureMo @Ligoml @luotao1

请评审!谢谢!

@paddle-bot
Copy link

paddle-bot bot commented May 28, 2023

你的PR提交成功,感谢你对开源项目的贡献!
请检查PR提交格式和内容是否完备,具体请参考示例模版
Your PR has been submitted. Thanks for your contribution!
Please check its format and content. For this, you can refer to Template and Demo.

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.

示例代码的量太大(近2000)段,有没有好的修改方法?或者搞个快乐开源让大家都来玩玩?!

可参考 中国软件开源创新大赛:飞桨框架任务挑战赛 赛事流程,本任务本来就是需要任务划分的,RFC 作者主要作为任务 Leader,承担任务的划分与分工,划分后建立 Tracking issue,发布给有兴趣的开发者来做,Tracking issue 可参考 PaddlePaddle/Paddle#53172 (comment) 列表里的内容,可以将这一点考虑进来写在 RFC 里~

Paddle docs 的 sphinx 文档编译还有不清楚的地方,PR 里面也写了,这里可否指导一下~

文档应该是用 https://github.com/PaddlePaddle/docs/blob/develop/docs-build.sh 构建的?细节我就不清楚了……

另外,RFC 非常详细!稍微修改下我觉得就没啥问题了 👍


## 3、意义

提高飞桨框架示例代码的质量。
Copy link
Member

Choose a reason for hiding this comment

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

这里多写一点吧,包括保证示例代码输出正确性等


[Paddle/tools/sampcd_processor.py](https://github.com/PaddlePaddle/Paddle/blob/develop/tools/sampcd_processor.py)

完成。
Copy link
Member

Choose a reason for hiding this comment

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

这种地方为什么要换行呢?

aaa
[bbb](ccc)
ddd

<!-- 改成下面的就可以 -->

aaa [bbb](ccc) ddd

行内代码块(`)一样,不需要换行,部分长命令可换成代码块(```)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这。。。 算是强迫症吧。。。 写成一坨看着难受。。。😂


此方法存在较多问题,比如,无法验证代码与示例中的结果是否一致,无法处理本应报错的示例代码等。

并且,Paddle docs 此处的检查与 Paddle 代码中的目标一致,属于对示例代码的重复检查,建议移除此检查。
Copy link
Member

Choose a reason for hiding this comment

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

这个我在一年前就有这样的建议了:

PaddlePaddle/docs#4973 (comment)

此外,由于我们现在示例代码已经基本全部采用 COPY-FROM 取代,因此也可以考虑取消 docs repo 里的示例代码检查,将示例代码检查完全交给 Paddle repo 里的 CI 来进行

@luotao1 觉得呢?

另外 @megemini 也可以统计下 docs 下目前还有多少的示例代码会被提取,多少是 fluid 下的


1. 前期准备阶段:Paddle doc 与 Paddle 代码 的检查共存,新旧代码样式共存,不进行代码示例的修改。

- 修改目前 Paddle docs 的代码检查方式,兼容 `google` 样式。
Copy link
Member

Choose a reason for hiding this comment

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

这里的描述是不是没有修改?还是之前方案的描述?应该主要修改 Paddle 这边 CI 的检查方式

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paddle docs 也要修改,如果不改的话,google 样式的示例,在 Paddle docs 中会报错。

参考后面 1.1 修改目前 Paddle docs 的代码检查方式 里面的示例在 Paddle docs 中目前的问题。

不过,如果直接移除 docs 中的检查,那就没啥问题了~

>>> x = paddle.to_tensor([-0.4, -0.2, 0.1, 0.3])
>>> out = paddle.abs(x)
>>> print(out)
Tensor(shape=[4], dtype=float32, place=Place(cpu), stop_gradient=True,
Copy link
Member

Choose a reason for hiding this comment

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

与 Torch 不同的是,Paddle Tensor 格式化后的字符串里有 place 信息,这个在不同平台上会不一样,这个是如何保证的呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

嗯,好问题,这个之前没注意到!

对于:

  • Tensor(shape=[2, 2], dtype=float32, place=Place(cpu), stop_gradient=True,
  • Tensor(shape=[2, 2], dtype=float32, place=Place(gpu:0), stop_gradient=True,

xdoctest 是认为不一致的。

目前看,可以从几个方面着手:

  1. 开发文档里面要写清楚

    比如,明确开发者在写示例的时候,是否关注 place

    • 关注:如特指 gpu 环境等,开发者应该写明:
      • +REQUIRES(env:gpu)
      • paddle.device.set_device("gpu")
    • 不关注:那么,默认环境为 “cpu”。此处为开发者认为示例检查的默认环境,而不是实际代码检查的环境。

    另外,xdoctest 还支持 ... 的方式,个人推荐这种方式,如:

    add_sample_code(
        globals()["abs"],
        r"""
    Examples:
        .. code-block:: python
    
            >>> import paddle
            >>> x = paddle.to_tensor([[0.1, 0.2],[-0.4, -0.2]])
            >>> out = paddle.abs(x)
            >>> print(out)
            ...
            [[0.10000000, 0.20000000],
            [0.40000001, 0.20000000]]
            ...
    """,
    )

    这样也是可以通过测试的。也就是说,在关注的输出内容前后,加上 ... ,这样 xdoctest 可以不比对前面和后面的内容。

  2. execute_xdoctestglobal_exec 加一个 paddle.device.set_device("cpu"),也就是说,每次执行前统一默认设置为 "cpu" 环境执行:

    def execute_xdoctest(docs):
        
        import xdoctest
        
        xdoctest_config = {
            "global_exec": r"\n".join(
                [
                    "import paddle",
                    "paddle.device.set_device('cpu')",
                ]
            ),
            ...
        }
        ...

另外,简单分析一下运行时状况(有 place 的示例代码):

  1. 不全局执行 paddle.device.set_device('cpu'),跟随运行时环境
执行环境 示例代码 检查结果
cpu 无REQUIRES 正常检查
cpu +REQUIRES(env:gpu) 和 paddle.device.set_device("gpu") 跳过检查
gpu 无REQUIRES 正常检查,如果有 place 则可能报错
gpu +REQUIRES(env:gpu) 和 paddle.device.set_device("gpu") 正常检查
  1. 全局执行 paddle.device.set_device('cpu')
执行环境 示例代码 检查结果
cpu 无REQUIRES 正常检查
cpu +REQUIRES(env:gpu) 和 paddle.device.set_device("gpu") 跳过检查
gpu 无REQUIRES 正常检查
gpu +REQUIRES(env:gpu) 和 paddle.device.set_device("gpu") 正常检查

这里看看怎么处理?之后我更新一下 rfc~

Copy link
Member

Choose a reason for hiding this comment

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

按照对比来看,「全局执行 paddle.device.set_device('cpu')」没有任何缺点?不过无 REQUIRES 的单测会不会在 gpu 环境下因为加了 paddle.device.set_device('cpu') 仍然跑 CPU?会不会因此而测试地不全面?

其实这里我有点好奇,我们的无 REQUIRES 的示例代码是否应该在 CPU 和 GPU 下各跑一次呢?如果需要的话,后者不太可行,只能选择前者,然后通过 ... 来解决两者之间的差异问题

cc @luotao1 @sunzhongkai588

Copy link
Contributor Author

Choose a reason for hiding this comment

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

当然有缺点:

  1. 如果执行了 paddle.device.set_device('cpu'),那么,真正需要 gpu 或其他环境的示例,则必须在示例中加入 paddle.device.set_device("gpu") 之类的代码,进行显性的转换。
  2. 对于一般的开发过程,不在乎是否为 cpu 或 gpu 环境,则很可能在一个 gpu 环境中执行代码,并将其输出拷贝到示例中。这样,在执行了全局 cpu 命令后,再执行示例检查则会报错!

这些问题,只能在开发文档中进行说明,约束开发者对于示例的写作~

如果要 cpu 和 gpu 环境各跑一次,并比对结果,那么,就只能用 ... 这个来解决兼容性问题了,毕竟:

  1. 一个输出不会同时有 cpu 和 gpu 两个结果
  2. 应该大部分开发者也不会写两遍示例代码,分别针对 cpu 和 gpu

所以,我觉得目前比较好的方案是:

  1. 加上 paddle.device.set_device('cpu')
  2. 开发文档中引导使用 ... 的写作方式

这样的话:

  1. 在 cpu 的执行环境中,那些 gpu 的示例代码由于不满足 REQUIRES 会跳过
  2. 在 gpu 的执行环境中,由于 paddle.device.set_device('cpu'),那些不依赖 gpu 的示例可以正确执行,而依赖 gpu 的示例也可以正常检查

以上,是不使用 ... 的情况,如果按照开发文档的指导,使用了 ... ,则不必关心执行环境了

不过无 REQUIRES 的单测会不会在 gpu 环境下因为加了 paddle.device.set_device('cpu') 仍然跑 CPU?会不会因此而测试地不全面?

针对这个问题,我个人的意见是,xdoctest 毕竟只是示例代码的检查,目的是防止示例 写错,而不是进行 单元测试,所以,应该允许部分的 漏测,如执行环境的不全面等~

Copy link
Collaborator

@jzhang533 jzhang533 Jun 7, 2023

Choose a reason for hiding this comment

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

这部分确实是个问题。但根本还是因为print paddle tensor,无法提供一个machine friendly的输出引起的。长期看,我觉得可以考虑让打印paddle tensor时,不输出place信息,其行为跟 pytorch/numpy 对齐会是比较理想的方案。

如果考虑兼容性的问题的话,就这个项目而言, 可以考虑增加一个内部API,调用该API,可以修改print paddle tensor的行为,让其不打印place信息。在进入xdoctest的时候开启该配制。

检查的环境的话,默认只在CPU上跑一遍就可以了。毕竟这个是示例代码的检查,不是单测。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这部分确实是个问题。但根本还是因为print paddle tensor,无法提供一个machine friendly的输出引起的。长期看,我觉得可以考虑让打印paddle tensor时,不输出place信息,其行为跟 pytorch/numpy 对齐会是比较理想的方案。

嗯~ 或者,有个类似 pprintpaddle.summary 之类的格式化输出的方法也行~

如果考虑兼容性的问题的话,就这个项目而言, 可以考虑增加一个内部API,调用该API,可以修改print paddle tensor的行为,让其不打印place信息。在进入xdoctest的时候开启该配制。

这个可能不太行~ 输出的比对是两方面,一个是开发者写的,一个是程序输出的,程序输出的我们可以控制,但是开发者如果用 print 输出并拷贝到示例代码中,还是会存在 place ~ 同上,长期看,最好是有一个两方一致的方法~

检查的环境的话,默认只在CPU上跑一遍就可以了。毕竟这个是示例代码的检查,不是单测。

赞同!

Copy link
Collaborator

Choose a reason for hiding this comment

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

但是开发者如果用 print 输出并拷贝到示例代码中,还是会存在 place

这里可以用CI监控下,示例代码中不能存在place。以下这个PR就是【跳过代码示例中print的使用监控】


**CAUTION: Review 注意**

> 由于 `registry.baidubce.com/paddleorg/docpreview` 这个 docker 镜像拉不下来,这里无法验证此方法的正确性!因此,此处特性的实现需要具体讨论!
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
Contributor Author

Choose a reason for hiding this comment

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

文档应该是用 https://github.com/PaddlePaddle/docs/blob/develop/docs-build.sh 构建的?

确实是,但是文档构建完之后,只是简单的 html,没有具体的显示格式~

registry.baidubce.com/paddleopen/fluiddoc-sphinx:20210610-py38 这个docker 里面,有个 preview 的脚本:

/root/fluiddoc-preview.sh

里面用到了这个 registry.baidubce.com/paddleorg/docpreview docker,以及 spring 服务器。

由于后面就没权限了,所以我猜测系统在具体呈现这些文档的时候,会再做一些处理。而加入代码复制的特性,可能就不在 Paddle docs 负责的范围内了~

所以,具体涉及到页面复制特性,以及sphinx构建相关的东西,我都写的比较少,还有待确认~

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个超出了本项目的范围了,可以不考虑。
等上线之后,请负责官网的研发同学来实现就好了。

Copy link
Collaborator

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

写的非常好!

  1. Paddle docs中API文档可以直接使用 COPY-FROM,基于这个前提,设计文档是不是要做一下调整,能省去很多后续成本。即我们可以先开一个任务把没有使用COPY-FROM的先做了,同时需要有CI检测,即docs中API文档不能引入示例代码

  2. 新旧代码风格的问题,实操上先把增量控制住,即新增/修改的API注释(这块CI是有的),必须要走google style(只需要改一下这儿的逻辑),不然CI报错。这样实操上是否简单点?

现有示例代码存量较大(近2000个),存在修改工作量风险。

这个能否开发一个脚本,自动补全 >>> ?,剩余补全不了的再人工修改。而且如果第2点的CI脚本上了,自动化改存量脚本也有CI保障,工作量可以收敛。

  1. 因为2.5版本已经拉分支了,我们目标在2.6版本前把这块做完。

@luotao1
Copy link
Collaborator

luotao1 commented Jun 7, 2023

@jzhang533 @sunzhongkai588 再review下

  1. 这样的实操步骤是否合理 [Add]将 xdoctest 引入到飞桨框架工作流中v1 #547 (review)

  2. 设备类型相关的采取哪个方案?[Add]将 xdoctest 引入到飞桨框架工作流中v1 #547 (comment)

我们的无 REQUIRES 的示例代码是否应该在 CPU 和 GPU 下各跑一次呢

@SigureMo 我猜测CI上只跑一次,但QA验收的时候都跑一下。

  1. 官网代码复制的特性是否先不用考虑?[Add]将 xdoctest 引入到飞桨框架工作流中v1 #547 (comment)

@jzhang533
Copy link
Collaborator

写的非常好!

  1. Paddle docs中API文档可以直接使用 COPY-FROM,基于这个前提,设计文档是不是要做一下调整,能省去很多后续成本。即我们可以先开一个任务把没有使用COPY-FROM的先做了,同时需要有CI检测,即docs中API文档不能引入示例代码

同意这点,因为理论上docs内的内容,是对英文版本的翻译,示例代码部分是不需要翻译的。

Copy link
Collaborator

@jzhang533 jzhang533 left a comment

Choose a reason for hiding this comment

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

非常赞的RFC,期待~


**CAUTION: Review 注意**

> 由于 `registry.baidubce.com/paddleorg/docpreview` 这个 docker 镜像拉不下来,这里无法验证此方法的正确性!因此,此处特性的实现需要具体讨论!
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个超出了本项目的范围了,可以不考虑。
等上线之后,请负责官网的研发同学来实现就好了。

@sunzhongkai588
Copy link
Contributor

  1. Paddle docs中API文档可以直接使用 COPY-FROM,基于这个前提,设计文档是不是要做一下调整,能省去很多后续成本。即我们可以先开一个任务把没有使用COPY-FROM的先做了,同时需要有CI检测,即docs中API文档不能引入示例代码

赞同开任务来清理没有copy-from的中文文档。目前基本上新增api的中文文档代码都是copy-from的,没有copy-from的都是比较旧的api文档。

现有示例代码存量较大(近2000个),存在修改工作量风险。

这个能否开发一个脚本,自动补全 >>> ?,剩余补全不了的再人工修改。而且如果第2点的CI脚本上了,自动化改存量脚本也有CI保障,工作量可以收敛。

我理解即使用脚本自动补全 >>>,可能还有大量的人工工作需要做。因为目前示例代码的输出结果有太多写法了,如rfc所述

另外,示例代码的格式虽然总体符合目前对于示例的要求,但是仍存在多种具体写法:

 - 输出与代码同一行

     ``` python
     # ISTFT
     x_ = istft(y, n_fft=512)  # [8, 48000]
     ```

 - 输出的注释与实际输出不一致

     ``` python
     p = lognormal_a.probs(value_tensor)
     # [0.48641577] with shape: [1]
     ```

 - 不使用 `print` 提示

     ``` python
     conv = paddle.nn.Conv1D(3, 2, 3, weight_attr=attr)
     conv.weight
     # Tensor(shape=[2, 3, 3], dtype=float32, place=CPUPlace, stop_gradient=False,
     #       [[[0., 1., 0.],
     #         [0., 0., 0.],
     #         [0., 0., 0.]],
     #
     #        [[0., 0., 0.],
     #         [0., 1., 0.],
     #         [0., 0., 0.]]])
     ```

 - 使用 `print` 提示

     ``` python
     res = linear(data)
     print(linear.weight)
     # Tensor(shape=[2, 4], dtype=float32, place=Place(gpu:0), stop_gradient=False,
     #        [[2., 2., 2., 2.],
     #         [2., 2., 2., 2.]])
     ```

这种简单、量大的工作可能还是主要得借助于「快乐开源」或者「LearnDL」

@megemini
Copy link
Contributor Author

megemini commented Jun 8, 2023

这两天有点事儿,抱歉回复的比较迟~

针对大家 @SigureMo @luotao1 @jzhang533 @sunzhongkai588 的意见,我这里先汇总一下,后续修改 rfc:

  • Paddle docs 中的 copy-from 问题。

    我设想,后续的整个流程:

    1. 开发者,在 Paddle 代码中,编写示例代码。Examples: 中,使用 .. code-block:: python 指令。
    2. 开发者,提交 Paddle 代码的 PR。
    3. Paddle 代码的 CI,执行代码检查(增量)。
    4. 开发者,在 Paddle docs 中,编写中文文档,使用 COPY-FROM: 指令。
    5. 开发者,提交 Paddle docs 的 PR。
    6. Paddle docs 的 CI,不做检查,直接根据 COPY-FROM: 复制示例代码。
  • paddle tensor place 的问题。

    如果根据 @jzhang533 的意见,在CPU上跑,那么,利用上面提到的 paddle.device.set_device('cpu') 全局命令,结合开发文档的引导,应该能解决大部分的问题了~

  • Paddle docs 中是否还需要做代码检查???!!!

    @SigureMo 也提到,Paddle docs 中的代码检查可以交给 Paddle 代码的 CI 完成~ 我觉得,后续如果 Paddle docs 都使用 COPY-FROM 指令,那么再检查代码确实是多余了~

    最重要的是,Paddle docs 后续是否需要代码检查,直接影响 rfc 和后续的开发任务,请务必先统一一下这个问题的意见!!!

    • 如果需要,那么,Paddle 代码 和 Paddle docs 需要两份 xdoctest 的检查,即使 Paddle docs 中都使用了 COPY-FROM,也会检查。
    • 如果不需要,那么,Paddle docs 前期可以只做规避(规避 google style 的引入,导致目前 Paddle docs 的检查出错),后续可以移除 Paddle docs 的检查。

  1. Paddle docs中API文档可以直接使用 COPY-FROM,基于这个前提,设计文档是不是要做一下调整,能省去很多后续成本。即我们可以先开一个任务把没有使用COPY-FROM的先做了,同时需要有CI检测,即docs中API文档不能引入示例代码

赞同!

这个确实应该开一个任务,我想,放在第二阶段第一个任务 分批次修改已有代码的示例,提交 PR 并通过检查。 之后。或者,在修改这些存量的时候,1对1的修改中文文档。

不过,不建议把这个任务放在最开始,因为,如果放在最开始,那么 Paddle docs 的修改还是要依赖 docs 自己的 CI 来检查!还是相当于要同时维护 Paddle 代码的 CI 和 Paddle docs 的 CI 两个部分。。。

  1. 新旧代码风格的问题,实操上先把增量控制住,即新增/修改的API注释(这块CI是有的),必须要走google style(只需要改一下这儿的逻辑),不然CI报错。这样实操上是否简单点?

嗯,目前 Paddle 代码 和 Paddle docs 中的 CI 都有增量检查,计划上是不改动增量/全量这部分的逻辑,只改动具体的检查部分~

考虑到 xdoctest 的引入,与开发者的引导之间,存在时间差,所以,我是想在流水线切换的这段时间,最好能兼容旧格式~

这里可以用CI监控下,示例代码中不能存在place。以下这个PR就是【跳过代码示例中print的使用监控】

我觉得,从开发者和学习示例代码的角度来看,如果目前示例中用 print 来输出,print 的输出也带有 place,那么,我们只能采用有限的手段来约束和检查,比如,指导开发者在写示例代码的时候,如无特殊情况,请用 CPU 环境,我们也在 CPU 环境中进行检查。

如果进行过多 magic 的控制,比如,限制示例代码中要删掉 place 部分,而学习者重现示例的时候,发现有 place,就会比较困惑。。。


以上,请评审!:)

@jzhang533
Copy link
Collaborator

  • Paddle docs 中是否还需要做代码检查???!!!
    @SigureMo 也提到,Paddle docs 中的代码检查可以交给 Paddle 代码的 CI 完成~ 我觉得,后续如果 Paddle docs 都使用 COPY-FROM 指令,那么再检查代码确实是多余了~

Docs仓库可以不要示例代码检查。

@luotao1
Copy link
Collaborator

luotao1 commented Jun 9, 2023

这个确实应该开一个任务,我想,放在第二阶段第一个任务 分批次修改已有代码的示例,提交 PR 并通过检查。 之后。或者,在修改这些存量的时候,1对1的修改中文文档。
不过,不建议把这个任务放在最开始,因为,如果放在最开始,那么 Paddle docs 的修改还是要依赖 docs 自己的 CI 来检查!还是相当于要同时维护 Paddle 代码的 CI 和 Paddle docs 的 CI 两个部分。。。

  1. Docs仓库可以不要示例代码检查,但Docs仓库还是需要有CI。
  2. 我建议是放在最开始,因为本身我们就推荐COPY-FROM,只是现在有一些没改过来,是一些历史遗留问题。其实可以说和框架引入xdoctest没有关系。
  3. 改COPY-FROM不需要动目前的两份CI,这是一个单独的历史遗留问题任务。

考虑到 xdoctest 的引入,与开发者的引导之间,存在时间差,所以,我是想在流水线切换的这段时间,最好能兼容旧格式~

  1. 开发者的引导,只要CI上有一个说明文档告诉他报错就可以了。因为改格式还是很简单的。
  2. 得看你兼容旧格式的成本有多大,会不会这个脚本变得很难维护?如果这样的话,可以考虑xdoctest引入后,只要动了文档,就必须使用新的示例代码格式(改成>>>还是很容易的)。

@megemini
Copy link
Contributor Author

megemini commented Jun 9, 2023

  1. Docs仓库可以不要示例代码检查,但Docs仓库还是需要有CI。

✨ 赞!!!

  1. 我建议是放在最开始,因为本身我们就推荐 COPY-FROM,只是现在有一些没改过来,是一些历史遗留问题。其实可以说和框架引入xdoctest没有关系。

✨ 同意!!!

那我单独开一个任务跟踪这个事情~

虽然不明白,为什么跟 xdoctest 没关系还要加到这个 rfc 里面 ~ 🤣 开个玩笑 别生气 ~ 🤭

  1. 改 COPY-FROM 不需要动目前的两份 CI,这是一个单独的历史遗留问题任务。

✨ 明白!!!

是不是可以这样做:

  • 修改 COPY-FROM 和引入 xdoctest 做为两条单独的任务线。
  • 保留目前 Paddle docs 中代码检查的 CI,这样可以为 COPY-FROM 的修改做检查(旧格式)。
  • 在 Paddle docs 代码检查的 CI 中做 google style 的规避(新格式)。(这么看,COPY-FROM 和 xdoctest 还是有点关系的 ~ 🤔)
  • 最后,在所有修改完成的某个时间点,移除掉 Paddle docs 中代码检查。
  1. 开发者的引导,只要CI上有一个说明文档告诉他报错就可以了。因为改格式还是很简单的。

✨ 收到!!!

  1. 得看你兼容旧格式的成本有多大,会不会这个脚本变得很难维护?如果这样的话,可以考虑xdoctest引入后,只要动了文档,就必须使用新的示例代码格式(改成>>>还是很容易的)。

✨ 可以!!!

这样的话就简单了:

  • 直接替换掉 Paddle 代码 CI 中的代码检查为 xdoctest。
  • 以此 PR 的合入为时间点,后面的示例统一用 google style,否则报错。

@SigureMo
Copy link
Member

SigureMo commented Jun 9, 2023

得看你兼容旧格式的成本有多大,会不会这个脚本变得很难维护?如果这样的话,可以考虑xdoctest引入后,只要动了文档,就必须使用新的示例代码格式(改成>>>还是很容易的)。

这里我是有一点担忧的,如果早期考虑的不够全面的话,直接替换很容易导致开发者不清楚如何去修改,有些写的明明是对的,但是不熟悉新的示例代码书写方式而难以合入,而且早期也是没有多少是可以参考的

如果不考虑兼容旧格式的话,可以考虑增加黑白名单,未迁移部分只跑旧的检查,迁移部分只跑新的检查,这样可以确保不会出现上述问题,而且也不需要兼容旧格式

在 Paddle docs 代码检查的 CI 中做 google style 的规避(新格式)。(这么看,COPY-FROM 和 xdoctest 还是有点关系的 ~ 🤔)

COPY-FROM 后的代码是不会检查的吧?目前代码检查应该是基于 rst 源码进行提取的,已经迁移到 COPY-FROM 的 rst 源码提取不到示例代码,应该是会直接跳过的

@luotao1
Copy link
Collaborator

luotao1 commented Jun 9, 2023

修改 COPY-FROM 和引入 xdoctest 做为两条单独的任务线。最后,在所有修改完成的某个时间点,移除掉 Paddle docs 中代码检查。

可以的。

为什么跟 xdoctest 没关系还要加到这个 rfc 里面

可以单独写一个Copy-FROM的RFC出来(然后我们就可以启动啦)。这样这个RFC更加精简和聚焦一点。都是你调研出来的成果呢。

如果早期考虑的不够全面的话,直接替换很容易导致开发者不清楚如何去修改,有些写的明明是对的,但是不熟悉新的示例代码书写方式而难以合入,而且早期也是没有多少是可以参考的。如果不考虑兼容旧格式的话,可以考虑增加黑白名单,未迁移部分只跑旧的检查,迁移部分只跑新的检查,这样可以确保不会出现上述问题,而且也不需要兼容旧格式

我建议这块可以出几个方案,用表格的形式对比一下优缺点:1)要兼容 2)不兼容的做法1(luotao)3)不兼容的做法2(SigureMo)4)不兼容的做法XXX。然后我们请大家来评审下。

@megemini
Copy link
Contributor Author

megemini commented Jun 9, 2023

COPY-FROM 后的代码是不会检查的吧?目前代码检查应该是基于 rst 源码进行提取的,已经迁移到 COPY-FROM 的 rst 源码提取不到示例代码,应该是会直接跳过的

@SigureMo 这个地方不太确定~

docs 的 CI 流程好像是:

  • docs/api/copy_codes_from_en_doc.py 根据 COPY-FROM 把代码复制出来。
  • docs/ci_scripts/check_api_cn.shcheck_copy_from_parsed_into_sample_code.py 检查是否有 COPY-FROM 转换出错的。
  • 如果没有,则用 chinese_samplecode_processor.py 检查代码。

提取不到的会跳过,没什么问题~ 提取到的好像还是要检查的吧?!

check_api_cn.sh

need_check_cn_doc_files="$1"
echo $need_check_cn_doc_files
# Check COPY-FROM is parsed into Sample Code
echo "Run COPY-FROM parsed into Sample Code Check"
python check_copy_from_parsed_into_sample_code.py "${OUTPUTDIR}/zh/${VERSIONSTR}/" $need_check_cn_doc_files
if [ $? -ne 0 ];then
    echo "ERROR: Exist COPY-FROM has not been parsed into sample code, please check COPY-FROM in the above files"
    exit 1
fi
need_check_files=$(filter_cn_api_files "${need_check_cn_doc_files}")
echo "$need_check_files"
if [ "$need_check_files" = "" ]
then
    echo "need check files is empty, skip chinese api check"
else
    echo "need check files is not empty, begin to install paddle"
    install_paddle
    if [ $? -ne 0 ];then
        echo "install paddle error"
        exit 5
    fi

    for file in $need_check_files;do
        python chinese_samplecode_processor.py ../docs/$file
        if [ $? -ne 0 ];then
            EXIT_CODE=5
        fi
    done
    exit ${EXIT_CODE}
fi

重现 COPY-FROM 的条件和环境挺麻烦的,不过,从逻辑上来看,目前的好像是这样,也没啥问题~ 还请帮忙看看!

@SigureMo
Copy link
Member

SigureMo commented Jun 9, 2023

提取不到的会跳过,没什么问题~ 提取到的好像还是要检查的吧?!

如果是这样,我认为提取到的完全没必要再检查一遍了,因为提取到的可以保证在 Paddle 那边已经检查过了,docs 再重新检查是完全没必要的,这个检查完全可以下线的

@megemini
Copy link
Contributor Author

megemini commented Jun 9, 2023

我建议这块可以出几个方案,用表格的形式对比一下优缺点:1)要兼容 2)不兼容的做法1(luotao)3)不兼容的做法2(SigureMo)4)不兼容的做法XXX。然后我们请大家来评审下。

  1. 要兼容

    Paddle 代码的 CI 中,在前期会同时有 新/旧 两种格式的检查,中期修改完开发文档后,统一切换到新格式。

    优点:

    • 流水线切换过程中,对于开发者比较友好。
    • 有时间对于示例中的各种情况进行梳理,整理成开发文档。
    • 对于示例代码的检查,存在一定的容错性,即,新格式的检查出问题(xdoctest 的引入出bug),旧格式还可以用。

    缺点:

    • 工作量多一点点。
    • 后续需要移除旧格式的检查。
  2. 不兼容(luotao)

    直接替换掉现有 Paddle 代码的 CI 中,示例代码检查的部分,为,引入 xdoctest 后的方式以及新格式。

    优点:

    • 流水线中只存在一种检查流程,分工明确。
    • 工作量少,后续不需要再移除旧格式的检查。

    缺点:

    • 对于开发者,在没有开发文档的引导情况下,会比较困惑。
    • PR 的审核过程中,对于各种情况,可能需要反复确认。
  3. 不兼容 (SigureMo)

    增加黑白名单,未迁移部分只跑旧的检查,迁移部分只跑新的检查,这样可以确保不会出现上述问题,而且也不需要兼容旧格式

    优点:

    • 不会出现新旧检查冲突的问题。
    • 还请 @SigureMo 补充

    缺点:

    • 我还没想好这种方案怎么做 ~ 😅

来,买定离手!!!🎲

@SigureMo
Copy link
Member

SigureMo commented Jun 9, 2023

我还没想好这种方案怎么做 ~

大概是按照目录分一下,如果一个目录已经迁移到新的格式,则加入到新格式名单中,不跑旧的检查,只跑新的检查,任何一个示例代码只跑一种检查

@luotao1
Copy link
Collaborator

luotao1 commented Jun 12, 2023

@megemini 因为评审意见比较多且格式检查涉及面较广,辛苦整理出一份Copy-From的RFC和xdoctest修改后的RFC,我们邀请更多RD来review下

@megemini
Copy link
Contributor Author

@SigureMo @luotao1 @jzhang533 @sunzhongkai588

统计 COPY-FROMcode-block 的过程中发现了点新问题,不在 代码示例 中的没法转换。

结合之前讨论的一些问题,更新了一版 rfc,请评审!

谢谢!:)

- 中英文 API 文档特性更新
- 代码检查移交

# 六、影响面
Copy link
Collaborator

Choose a reason for hiding this comment

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

这一部分【影响面】,做成表的形式更加精简,便于阅读

Copy link
Contributor Author

Choose a reason for hiding this comment

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

改了,没法看,又改回去了 😵‍💫

- 更新文档《开发 API Python 端》与《API 文档书写规范》
- 不再兼容旧格式
- 中英文 API 文档特性更新
- 代码检查移交
Copy link
Collaborator

Choose a reason for hiding this comment

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

排期规划

  • 需要大致写一下工作量和人员安排,可以用表的形式。比如 1661行和1664行需要借助快乐开源任务,其他是你自行开发?
  • 哪些工作可以并行开展?

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` 对应的 API

`paddle/fluid/API_PR.spec`
Copy link
Collaborator

Choose a reason for hiding this comment

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

155行-161行,可以合并成一行么,这样RFC看起来更加精简。或者是出于什么考量分开写呢?全文多处是这样的。

Copy link
Collaborator

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

其余我没什么问题了~


> **Xdoctest 示例样式说明**
>
> - A `Google-style <https://sphinxcontrib-napoleon.readthedocs.io>`__ doctest is
Copy link
Member

Choose a reason for hiding this comment

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

这个链接是从 reStructureText 里 copy 过来的?需要改成 Markdown 格式

>
> - A `freeform style` doctest is any contiguous block of lines prefixed by ``>>>``.
This is the original parsing style of the builtin doctest module. Each block is
listed as its own test.
Copy link
Member

Choose a reason for hiding this comment

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

我看这次更改主要将 google style 替换成了 google/freeform style,这里是怎么考虑的呢?按理说我们的英文文档本来就应该是符合 google style 的,为什么还需要 freeform style 呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

freefrom 与 google 样式的最大区别是,freeform 会解析 docstring 中所有 >>> 中的代码,google 样式只解析 Example 内的代码~

因为 google 样式其实是指 docstring 中的 Args: Returns: Examples: 这样的写作格式,而不管是 google 还是 freeform 都应该符合 >>> 这种 doctest 的写作方式,而不是我们现在这种直接写代码的方式。

v1.2 这个版本最大的更新,是在统计 docs 示例代码的时候,发现有些代码是不在 Example 里面的,参考 ### 3.2 Paddle docs 以及第一个任务 修改目前 Paddle docs 中 COPY-FROM 的逻辑

所以把 google style 替换成了 google/freeform style,表示既需要检查 Example 里面的代码,也要检查其他部分 docstring 中的代码。

坑越挖越大 ... ... 💔

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.

LGTM

@luotao1 luotao1 requested a review from jzhang533 June 15, 2023 07:37
Copy link
Collaborator

@jzhang533 jzhang533 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 14e7808 into PaddlePaddle:master Jun 15, 2023
1 check passed
Copy link
Contributor

@sunzhongkai588 sunzhongkai588 left a comment

Choose a reason for hiding this comment

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

LGTM~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants