代码审查

开源代码由具有不同背景、兴趣和目标的社区维护。因此,提供清晰、有文档记录且可维护的代码和流程非常重要。代码审查是一种引导过程,用于集体发现潜在问题,提高代码质量,并教育贡献者和审查者关于代码库及其假设。它也是确保有多人可以共同维护相关代码的一种机制。鼓励贡献者在请求审查之前将代码打磨到可审查状态。对于 Committer 候选人来说尤其如此,因为 Committer 不仅需要编写代码,还需要参与审查代码。

本文档是开源代码审查的动态指南。请花一些时间阅读 TVM 社区准则,了解一般的开发流程。

建立信任

首先,我们正在建立一个基于信任的社区,这需要时间和精力来建立和维护。我们期望我们的社区成员以建设性的方式协同工作,并运用常识一起工作。尽管我们都有不同的背景、兴趣和目标,但我们必须共同努力,找到对更广泛的社区有效解决方案。基于信任的协作也是 Apache 之道的核心原则,是在社区发展和晋升成员担任官方角色时需要考虑的重要因素。

社区参与

欢迎所有人对 PR 发表评论。我们鼓励 Committer 在合并包含重大架构更改的 PR 之前等待一段时间(例如三天)。目的是给人们时间发言,表达对审查和参与的兴趣。

记住我们都来自不同的背景在这里很重要。例如,一些社区成员在不同的时区工作,仅在工作时间内从事开源工作,或者可能正在旅行或生活中发生其他事件。大型项目中工作的一个重要部分是确保有集体的理解,这样就不会有一个人成为瓶颈。虽然允许时间参与代码审查很重要,但我们也不能阻止所有审查者的所有更改。请记住,帮助人们完成 PR 是鼓励更广泛参与的好方法,特别是对于那些志愿贡献时间的人。

这部分是信任并与同为维护者的其他人沟通,如果将来需要应用更改,PR 作者稍后会兑现他们的承诺。 Committer 有责任倾听所有反馈,无论是来自 PMC 成员还是新贡献者的反馈,并考虑需要采取哪些行动。

仔细阅读代码

有时我们可能会快速浏览代码,只注意到代码的某些方面。这些类型的评论通常很有帮助,应该在社区中受到欢迎。但是,它们只是执行代码审查的一部分,应该是更全面的反馈的一部分。良好且仔细的代码审查需要大量的时间投入,有时甚至比编写实际贡献的时间还要长。

例如,仅收到对您的 PR 的次要方面的高度批评性反馈很少让人感觉良好,如果您的时间和精力在审查期间没有得到回报,可能会让人感到沮丧。在作为贡献者和 Committer 行事时练习同理心非常重要,并且可以帮助您成为更有效的代码审查者和贡献者。

我们期望所有 Committer 在签署之前仔细阅读并理解代码。当 Committer 点击合并按钮时,其中涉及很多信任。与此同时,我们承认有时问题会溜走,在这种情况下,合并者有责任确保采取正确的后续行动。

保持尊重

  • 对于所有发表评论的人:提出建设性的评论将帮助新的贡献者及时完成他们的 PR,并帮助我们欢迎新成员加入社区。

  • 对于作者:审查者应该花费大量时间阅读代码,仔细的审查可能与从头开始编写代码一样耗时。尊重地回应审查意见,并在未来通过帮助审查其他人的更改来回报审查。

最重要的是专注于进行建设性的对话,并在作为审查者进行互动时尝试假设最佳意图。如果流程中存在问题,请考虑与其他贡献者进行面对面的交流,并讨论如何改进流程或沟通。

代码质量的考虑因素

高质量的代码对于项目的长期成功至关重要。在代码审查期间需要考虑许多代码质量因素

  • F0:总体架构。这包括公共模块、关键数据结构和公共接口的定义。良好的架构选择对于项目的长期成功至关重要。

  • F1:架构一致性。通常有多种方法来实现新功能。我们必须确保新功能与以前的总体架构选择保持一致,并与现有代码良好地交互。每个新功能都会增加项目的复杂性,一致的设计理想情况下可以最大限度地减少新功能带来的复杂性增加,从而使代码在长期内更易于维护。

  • F2:代码鲁棒性和测试覆盖率。确保代码在所有可能的设置(平台)中正确运行,确保新功能的测试覆盖率。用户面临的错误有清晰的错误消息。

  • F3:面向用户的 API 文档:强制要求公共面向用户的 API 和关键模块接口的文档。这包括 API、公共接口中出现的数据结构(即,include/tvm 和面向用户的 python API)。我们通常鼓励良好文档记录的代码,并为在多个地方使用的内部 API 包含某种形式的文档,另请参见 F4。

  • F4:代码可读性。可读性涉及多个方面:有指导意义且一致的函数名称、总体流程的清晰实现、复杂代码逻辑和内部函数的描述性注释。可读的代码更易于维护。

架构设计和一致性是最重要的因素,因为它们最有可能引入长期的技术债务。因此,Committer 应该在合并代码之前最仔细地考虑这些因素。

代码贡献需要测试覆盖率和 API 文档。

与其他因素相比,代码可读性相对而言是一个主观问题。不同的人对如何最好地编写代码有不同的想法。审查者应提出建设性和可操作的意见。与此同时,代码审查不应被用作让其他人按照您的方式编写代码的方式。相反,您还应该考虑到您可能容易理解或认为可以接受的内容可能不适用于更广泛的社区或其他成员。根据贡献的内容和范围以及贡献者的来源,使用您的判断来确定什么是合适的。

我们在编写代码时遵循通用的代码指南和技巧。风格指南有助于确保代码在原始作者离开很久之后仍然可读和可维护。风格指南不仅仅是关于代码格式化,它们还涉及正确记录代码、变量命名以及自动格式化程序无法强制执行的其他约定。

达成共识

代码审查期间可能会发生分歧。我们鼓励在相关人员之间达成共识。我们正在共同努力,并在 OSS 中相互建立信任。 OSS 的性质意味着有时我们会在不太重要的问题上做出妥协,以取得稳步进展并欢迎更广泛的社区参与。不幸的是,妥协意味着有时世界不会完全如我们所愿,即使对于社区领导者来说也是如此。

  • 保持文明,并通过基于技术的建设性对话建立共识。

  • 拥有该领域的 Committer 可以作为引导者,通过考虑所有对话来推动讨论,并提出解决方案以向前推进。

  • 由于 Committer(引导者)承担了很多信任,他们应该在签署之前仔细阅读 PR。此外,合并者还应负责在合并导致问题时进行跟进。

一致性

最后要说的是,我们都是人,很难始终保持完美的一致性。如果贡献者觉得您没有以一致的方式应用这些准则,那么倾听并听取大家的意见非常重要。随着我们作为一个社区的发展,我们将不断迭代流程和指南。我们的目标是努力保持一致和客观,但我们所有人不幸都是不完美的人,需要调整和学习。

其他建议

认真考虑 API 和数据结构

最小且稳定的 API 对于项目的生命至关重要。一个好的 API 会产生巨大的影响。始终非常仔细地考虑所有方面,包括命名、参数定义和行为。

在可能的情况下,在代码审查期间更加关注提议的 API 设计。请记住,改进代码实现更容易,但一旦接受 API,就极难更改 API。我们应该以相同的方式对待跨模块共享的数据结构(例如 AST)。如果不确定,请在提交之前与更多开发人员进行对话。

以下是一些有用的 API 设计原则

  • 如果功能重叠,请与现有著名的软件包的 API 保持一致。例如,张量运算 API 应始终与 numpy API 保持一致。

  • 与同一项目中的现有 API 保持一致。例如,我们应该在所有优化Pass中使用相同的参数顺序,这样在使用它们时就不会有“意外”。

  • 考虑 API 未来是否会更改。例如,随着我们在构建中添加更多优化,我们将有更多选项,如 loop_unrolling 和设备放置策略。我们可以将优化旋钮打包到构建配置对象中。这样,即使构建 API 可能会得到丰富,但它在一段时间内也是稳定的。

  • 编写文档。文档对于 API 是强制性的,有时编写文档有助于我们进一步思考设计,以及我们是否需要添加进一步的澄清。

  • 最小化。考虑用户必须编写多少行代码才能使用 API。尽可能删除抽象层。

最小化依赖

在引入依赖项时始终要谨慎。虽然重用代码和避免重复发明轮子很重要,但依赖项会增加用户在部署中的负担。一个好的设计原则是,只有在用户实际使用功能或函数时,该功能或函数才应该具有依赖项。

简洁的实现

这里应用一些基本原则:优先使用向量化数组代码而不是循环,使用解决问题的现有 API。

记录代码审查中的经验教训

当您发现可以总结一些常见或重复出现的经验教训时,请将其添加到代码指南和技巧中。在请求更改时,始终最好参考指南文档,以便可以将经验教训分享给整个社区。

从其他代码审查中学习

可能有多位审查者审查相同的更改。很多时候,其他审查者可能会发现您没有发现的东西。尽可能尝试从其他代码审查中学习,并记录这些经验教训。

明确批准和请求更改

贡献者和代码所有者可以向多位审查者请求代码审查。当您的评论在代码审查中得到解决时,请记住批准更改。为此 - 请单击拉取请求中的“更改”选项卡,然后选择“批准”,或评论代码并单击“请求更改”。如果某些审查者没有及时回复(例如一周),并且现有审查已足够,则代码所有者可以根据具体情况决定是否可以合并代码。

审查者

审查者应努力对请求其审查的拉取请求及时留下反馈。审查代码是项目健康的重要组成部分,应被视为贡献者的常规责任。自动化工具在这方面有所帮助,因为对于一段时间内没有活动的 PR,机器人会评论并 ping 相关方。