跳到内容

代码审查

本文档介绍了 BioNeMo 仓库中代码审查的流程和礼仪。如果您是在 BioNeMo 仓库中工作的开发人员，则应阅读本文档。

这些指南的目的是帮助减少编写代码的工程师和审查代码的工程师之间的摩擦。与许多规则一样，也有例外。这些例外并非详尽无遗，因此如果您发现应列出的例外情况，请提出，以便可以评估是否应将其纳入。

代码审查流程

代码审查流程是渐进的

  1. 您的团队审查
  2. 领域专家（CODEOWNERS）审查
  3. Approval-list 用户批准。
  4. （可选）Approval-list 用户进行覆盖率检查批准。

1. 团队审查

您应该首先要求贡献者审查您的更改。贡献团队可以提供最符合上下文的反馈，此审查步骤应该可以发现和解决更改中的大多数问题。

在您解决团队的意见并收到批准后，继续执行下一步。gitlab 中没有实际要求必须获得团队的批准 - 这只是一种最佳实践。

2. 负责人审查

代码负责人是仓库特定部分的领域专家。他们通常是一组源文件的原始作者。代码所有权结构往往反映团队结构；单个团队通常负责一个功能组件。

您必须获得 至少一位 每个 您更改的文件的负责人的批准。除非文件在 CODEOWNERS 文件中未指定任何负责人。

如果您的更改仅修改您团队拥有的文件，则负责人审查会在团队审查步骤中隐式发生（即，在许多情况下，您可能在上面的步骤 1 之后已经满足此要求）。

3. 批准

您还必须获得更改的批准。批准表示更改已准备好合并，并且是审查过程的最后一步。严禁自我批准。

批准由批准者授予，形式为 gitlab 中的批准章。批准者审查整个更改，而负责人则侧重于审查他们拥有的文件。

要查找批准者，请首先询问您的团队是否有首选的批准者。您也可以在 #clara-discovery-bionemo-dev 中查看。

*如果您的团队有批准者，通常会在您的团队有机会审查您的更改后不久获得批准。

*通常，一个人的单次审查就足以完成整个审查过程。如果审查者在您的团队中，是批准者，并且是您修改的文件的负责人，则该过程可能会简化为单次审查。

4. 覆盖率检查

我们的仓库具有自动检查功能，以确保测试覆盖率不会降低。覆盖率检查批准者将与 Approval-list 用户相同。如果代码行测试覆盖率降低，则批准者必须判断合并代码是否可以接受。有时，覆盖率检查算法会出现误报（即，代码覆盖率未降低，但 gitlab 标记了覆盖率检查批准），在这种情况下，批准者可以自由地批准错误的覆盖率降低。

职责

所有评论者（审查者、负责人、批准者等）

如果任何人启动了评论线程，则期望 线程启动者解决评论。原始线程启动者解决线程表示启动讨论的人对结果感到满意。

审查者

所有开发人员都可以并且应该审查更改。这些审查对于维护代码库的质量至关重要。审查也是了解人们正在从事什么工作并提高代码区域的巴士系数的重要方法。

审查者在审查代码时应始终执行以下操作

  • 保持尊重。

  • 假设最好的意图。

  • 审查代码，而不是人。

  • 留下清晰、可操作的反馈。在 gitlab 中使用评论来表示您希望进行更改，并明确列举它们。如果您不这样做，可能会让补丁的作者怀疑您的评论是否是可选的。请求代码更改不应被解释为对更改的判断，而应被解释为审查者希望与作者互动以将其转变为批准的指示。

更详细的礼仪指南可以在本文档的后面找到。

负责人

作为负责人，您有权通过拒绝签字来延迟代码合并。这种能力伴随着重要的责任

  • 及时回复审查的义务。如果您无法及时处理审查请求，则应放弃所有权。

  • 偏向“是”。“否”本身 绝不是 可以接受的答案。当您拒绝更改时，您必须与更改作者合作以达成双方都同意的解决方案。

批准者

作为批准者，您除了负责人之外还有其他责任

  • 您对更改及其对代码库的影响负责。

  • 您必须确保已由适当的审查者执行了审查，并且这些审查不是匆忙完成的。

  • 如果更改破坏了某些内容，则您应该积极参与清理工作。

如果您不熟悉更改所涉及的代码区域，则应始终拒绝批准更改。

成为负责人

代码所有权信息存储在 CODEOWNERS 文件中。由于这些 CODEOWNERS 文件存储在仓库中，因此更改它们的过程与更改代码的过程相同。

要成为负责人，请将您的 github 用户名添加到您想要成为其一部分的 CODEOWNERS 文件中，并将更改提交到 Gitlab。对 CODEOWNERS 文件的更改将遵循与任何其他代码更改相同的审查流程。现有负责人将决定是否将所有权委托给您。

成为批准者

作为批准者，您负责确保我们代码库的一致性和完整性。在成为批准者之前，请学习本文档，以便您完全熟悉审查者和批准者的责任。此外，请确保您非常熟悉我们的编码风格指南和最佳实践

良好公民的礼仪详情

在以下部分中，我们为每个仓库贡献者提供更深入的思考和建议，以便团队成员之间进行富有成效的互动。

补丁更改、大小和策略

  • 审查每 100-200 行代码需要 30-45 分钟。请注意您引入的更改的大小，并尽可能将补丁保持在 500 行代码以下。随着更改大小（代码行数）的增加，审查者发现问题的能力会降低。

  • 作为推论，对于重构类型的更改，请考虑将“无逻辑更改”和“逻辑更改”分成不同的审查（可能在相关的审查更改中），以减轻审查者的模式匹配负担。

  • 如果 PR 超出 500 行净代码添加量，所有审查者都可以要求 PR 过大。唯一的例外是进入 bionemo2/contrib 目录的 MR，其中允许更大的 MR。这包括代码行，但不包括虚拟数据或虚假数据集，这些数据或数据集可能包含数千行非实际功能代码的内容。

  • 每个补丁应保持为一个逻辑更改，该更改应在补丁的标题中描述。不相关的更改应拆分为单独的补丁。修复您正在编辑的行上的空格是合理的。修复您正在处理的代码周围的空格应是一个单独的“清理”补丁。

  • 在可能的情况下，较大的补丁（>500 LOC）应拆分为多个较小的补丁，这些补丁在个体上是一致的。在通过 gitlab 管道或本地方式将补丁提交到 Gitlab 之前对其进行测试。您在提交补丁以进行审查之前测试得越多越好。如果您在提交消息中添加一行来描述补丁的测试方式，也将不胜感激。这可以防止人们不得不询问补丁是否以及如何进行测试。说明补丁未经过测试也可以，尽管在合理的情况下，您可能会被要求进行一些测试。

  • 放弃不再有用或您不打算继续处理的补丁。

  • 遵循项目文档中声明的代码样式和规则（例如，contributing.md，其中 Google Python 风格指南 是子集），因为这些定义了代码的外观和风格，这定义了如何开发代码的最基本原理，并允许审查者专注于新代码的最重要方面。对于 bash 脚本，请遵循 此处 的 Google Shell 风格指南

  • 对于任何由于 PR 引入了完整性检查未捕获的错误而可能出现的严重错误，我们在代码库中遵循还原 + 修复策略。在 MR 无法还原且热修复已准备就绪的特殊情况下，领导层可以考虑在不还原的情况下合并。但是，这应该是一个例外。故障策略在 此处 中进行了更深入的描述。

审查时间表

  • 通常，自上次对更改进行重大修改以来，补丁应保持打开以供审查至少 24 小时。目的是让世界各地的开发人员有机会进行审查。复杂的返工，即使它们不改变补丁的目的，但改变了它的实现方式，也应重新开始等待期。

  • 快速更改：如果更改的目的是修复最近引入的、无法还原的问题，则更改可以在没有等待期的情况下进行。在这种情况下，提交消息必须解释哪个更改引入了问题以及问题的性质，以便紧急需求变得显而易见。更改本身应尽可能限制范围和影响，使其易于评估影响。

  • 处理空格或拼写修复等不影响最终二进制输出的次要问题的琐碎更改也不需要等待世界范围的审查。此类更改应在其提交消息中指出作者如何验证二进制输出是相同的。请注意，琐碎的修复不一定需要加快速度：就像它们对于因它们而出错而言不够关键一样，它们对于需要快速处理而言也不够关键。

审查者

  • 如果您没有审查被要求审查的代码，请勿批准。花时间审查或重新分配给其他人。批准更改的人员必须全面审查整个更改。如果您是特定文件的代码负责人，则仅审查您拥有的文件是合适的。

  • 如果请求 PR 更改其代码，您有责任为可以更改哪些内容以解决补丁解决的问题提供具体建议。如果您强烈认为永远不应合并补丁，则您有责任捍卫自己的立场并听取其他观点。要求更改后一走了之是不可接受的，并且可能导致您的批准状态被领导团队撤销。

  • 包含评论理由：当您审查代码时，您应始终尝试包含对您的评论的理由，除非该评论是吹毛求疵、违反样式指南或明显的错误。吹毛求疵和违反样式指南往往重叠，您不应必须证明使用共享样式指南或正确的拼写等是合理的。同样，您不应必须证明无错误代码是合理的。但是，当涉及到设计选择时，您应始终包含理由，说明您何时可能想要更改某些内容

  • 如果补丁上有评论或讨论，请在批准之前验证评论是否已得到解决。如果您认为评论无效，请回复该评论，而不是直接忽略它。

  • 批准补丁时要认真负责。作为仲裁者，您需要确保 MR 是完整的，已完成适当的审查，所有必需的测试都已通过，API 更改已由 API 负责人审查。请确保必要的收敛测试和单元测试已通过且未降低。如果发现 KPI 降低（收敛测试），您可能需要在批准 MR 之前咨询其他利益相关者。在某些情况下，Cl 将需要收敛测试。负责人/批准者应确保在必要时执行收敛测试，并且未发现任何新问题。并且总体而言，代码更改是合理的，并且与模块和系统的其余部分很好地集成。如果补丁破坏了某些内容，则您应该积极参与清理工作，并通过还原和加速修复来支持作者。这意味着您不应仅仅因为信任补丁的作者就批准补丁 - 确保您了解补丁可能产生的影响，或者将审查留给其他人。部分审查（例如，审查代码风格）可以给出正面评价或 LGTM。如果您认为补丁看起来不错，但可能没有经验知道是否存在意外后果，这也适用。

  • 请确保代码更改已包含在现有单元测试中，并在必要时要求贡献者添加或更新测试。

贡献者

  • 在提供补丁进行审查之前，请问自己以下问题

  • 此 PR 的大小是否合适？如果太长，请将其分解。

  • 此 MR 和包含的所有更改是否必要？（所有代码都必须维护）

  • 此 MR 是否重复现有功能？如果是，我可以扩展现有的功能吗？

  • 代码是否可读？我是否使用了影响可读性的深奥语言结构？代码是否遵循代码库的约定？

  • MR 是否已准备好投入生产？换句话说 - 它是否具有测试、文档、错误处理等。

  • 引起人们注意您希望审查的补丁。添加审查者，在 slack 上请求审查者，甚至只是将其基于当前代码库重新构建，以使其位于 Gitlab 列表的顶部。如果您不确定谁是合适的审查者，gitlab 将根据 UI 中的 CODEOWNERS 文件建议负责人，或者查看您已更改的文件的 git 历史记录，并添加这些人。对于基于 NIM-API 的更改，有一个小团队管理这些更改，因此请寻找这些人来审查 API。

  • 当更改您不拥有的区域时，请尝试与代码的其他重要贡献者协调。在您键入一行代码之前！这些人对代码的该部分进行了最重要的更改，因此了解任何需要权衡的因素。在已经编写新代码的情况下进行沟通将导致痛苦的来回，并且对所有人来说效率都会降低。了解设计，提出更改并在进行更改之前达成协议。

  • 除非您已与该分支的所有者协调，否则请勿修改其他人的分支。这不仅被认为是不礼貌的，而且您的更改可能会意外丢失。一个例外是超过 90 天未更新的分支，因此可以被视为孤立的分支。

  • 回复任何花时间审查您的分支的人，即使只是说您不同意。虽然解决修复拼写或“琐碎”问题的请求可能看起来很烦人，但在 Gitlab 的内置编辑器中通常很容易处理。如果您确实使用了内置编辑器，请记住在重新推送之前将该更改同步到本地副本。也可以将这些类型的评论的修复添加到另一个分支，但建议在该初始分支提交之前将该分支推送到 Gitlab。

  • 检查是否需要更新文档以在您的更改后保持最新状态。如果您正在处理的部分没有文档，请考虑添加一些文档。

  • 当对代码库的核心部分进行重大更改，或者当引入您认为值得在整个树中应用的新方法时，请将您的设计文档添加到提交中，以便审查者可以阅读它。

  • 除非您要求人们审查您的补丁，否则不要期望人们会审查它。添加其他人作为审查者是最简单的方法。但是您也可以使用 Slack 主动 ping 审查者，这对于紧急 MR 尤其有用。

  • 不要期望人们放下他们正在做的一切来审查您的补丁。每个人都有白天的工作，虽然代码审查是该工作的一部分，但它们通常不会延长整个工作日。

  • 不要解决（ack/完成）其他人打开的任何评论，以便他们可以轻松找到他们的评论并解决它们。除非被告知要自行解决。

  • 作为贡献者，您对您引入的代码以及在单元测试或收敛测试期间捕获的任何故障负责。等等。您应该监控您的代码是否已成功合并，以及代码合并后是否出现任何错误（夜间测试/收敛测试），并及时响应以解决任何故障。

通用礼仪

  • 我们试图在这个社区中假设彼此的善意。讨论错误是可以的（例如，早期提交的孤立的非琐碎和非关键更改实例），但尽量保持这种询问不带指责。如果更改导致代码出现问题，则重点应放在解决问题上，而不是追究责任。

  • 在评论分支时，请尊重他人。评论应仅限于代码，并应保持礼貌的语气。假设您的同事是聪明的，并且没有任何恶意或不尊重。抵制对感知的言语不当行为进行报复的冲动，这种行为不利于分支的合并。此外，避免绝对和具有攻击性的语言，因为这可能会加剧情绪。评论旨在进行协作。很多时候，评论者也可能是错误的，因为他们可能不了解补丁的全部情况，因为他们不是作者。评论不是命令，而是作者和审查者之间朝着双方都接受的解决方案提出的建议。

  • 不要提交您知道会破坏其他平台/依赖项的代码。如果您的补丁影响了其他人使用的代码，则它应该与这些代码兼容。虽然更新任何其他依赖项会很好，但您至少必须提供一条允许其他平台继续工作的路径。

  • 请勿撰写模糊不清或合作伙伴阅读日志时无法理解的提交信息。例如，请勿在提交信息的标题中写“\[主题] Bug修复”之类的标题。提交信息中不要包含视频链接。再次强调，合作伙伴将会看到这些日志，链接到他们无法访问观看的内容是没有意义的。请将您的提交信息每行控制在 72 个字符以内。好的提交信息应包含适当的主题，并用简洁的一句话解释更改内容。然后，对更改进行几句或更详细的描述。请使用专业的语言，不要在信息中包含合作伙伴无法理解的内容。避免使用首字母缩略词、缩写或代号，尤其是那些仅在英伟达内部有意义的词汇。此外，请使用正确的英语（请检查您的拼写）。这适用于分支合并后的最终提交信息。中间提交信息无关紧要。提交必须在合并时进行 squash 操作。

  • 考虑将大型的独立补丁分解成按区域分组的较小补丁。这使得补丁更容易审查，但会增加补丁的数量。如何处理这是一个个人决定，只要每个补丁仍然是一个逻辑上的更改即可。

  • 对 bionemo/contrib 文件夹所做的贡献具有更灵活的要求。所有未在下面提及的要求仍然适用于 contrib 文件夹。以下是例外情况：

  • 代码行数限制增加到 2200 行。

  • 单元测试覆盖率要求降低至 65% 覆盖率。
  • 代码行长度 <=120 个字符空格是可以接受的。

  • 提交信息不需要像以前那样有详细的解释。所有其他与批准人、贡献者和审查者责任相关的要求仍然适用。

  • 审查是关于代码的。当有人批评你的代码时，很容易感到个人受到冒犯，但整个想法是为了将更好的代码引入我们的代码库。同样，这也适用于另一个方向：审查代码，批判性地思考并回应代码，但不要将其个人化。

  • 不要在个人分支上长时间开发，然后将大量文件和代码行作为新的“功能”转储到审查中。功能的开发应该像任何其他更改一样，基于主干进行。随着代码变得越来越关键，遵循流程就显得尤为重要。功能可以分解为小的可工作更改，无需一次性全部开发完成。创建如此大的更改会导致审查过程效率降低，并可能导致更多错误。

  • 在 BioNeMo 中，对于正在开发中但尚未准备好投入生产的功能，我们将其放在 bionemo2/contrib 文件夹中。这允许团队在测试代码的同时更快地开发（审查不那么严格）。当一个功能完成并通过充分测试后，我们将其移动到 bionemo2/src 或 bionemo2/core，并完成所有生产要求的步骤。

参考文献