RFC: Add Pull Request rules to oneDAL#3264
RFC: Add Pull Request rules to oneDAL#3264icfaust wants to merge 17 commits intouxlfoundation:rfcsfrom
Conversation
icfaust
requested review from
Alexandr-Solovev,
Alexsandruss,
Vika-F,
keeranroth,
napetrov,
rakshithgb-fujitsu and
syakov-intel
|
@icfaust Text has redundant spaces after each period. |
|
|
||
| ## Preamble: | ||
|
|
||
| This is the rules for merging PRs into the oneDAL code base on Github. This |
There was a problem hiding this comment.
| This is the rules for merging PRs into the oneDAL code base on Github. This | |
| Thre are the rules for merging PRs into the oneDAL code base on Github. This |
| 1. PR Template and Task Checklist | ||
| * All PRs must use the provided template, including the task checklist. | ||
| 2. Draft PRs | ||
| * PRs should initially be opened as Draft. |
There was a problem hiding this comment.
I think this can be skipped in many cases, like changes to readme files and docs.
There was a problem hiding this comment.
Agree that for many cases this would be optional
| * PRs should initially be opened as Draft. | ||
| 3. Marking PRs as Ready for Review | ||
| * Before marking a PR as Ready for Review: | ||
| * All tasks in the checklist must be completed. |
There was a problem hiding this comment.
It's missing the part about removing inapplicable lines from the checklist.
| 3. Marking PRs as Ready for Review | ||
| * Before marking a PR as Ready for Review: | ||
| * All tasks in the checklist must be completed. | ||
| * Features and changes must be fully implemented. |
There was a problem hiding this comment.
This is usually not the case for large features - things are split into multiple parts, leaving some things out of the initial PRs. And some (particularly around non-x86 builds) remain yet unimplemented.
There was a problem hiding this comment.
Thats a good point. I would like to improve speed to merge, and large PRs should be discouraged. How would you recommend we re-word this?
There was a problem hiding this comment.
I would suggest that something is ready to go in if it either passes existing tests (where changing current functionality), or passes new tests added for new functionality. Maybe wording like the following?
* Where new functionality is added, new tests should also be added to test the functionality. If a situation arises where new code is not tested, or cannot be tested, clear justification must be given in the PR as to why this is the case.
I think that follows on from the next bullet point about CI passing, and should come after it.
With those changes to the wording, that satisfies as what I see as a 'fully implemented' change. One that does not break existing functionality, and that proves (apart from exceptional cases) that new code works.
| 2. Removal of CI tests can only be allowed with the written and documented | ||
| consent\*\* of the code owners in extreme circumstances. | ||
| 3. Code which causes new CI failures or problems should apply additional | ||
| fixes within a day, otherwise the PR should be reverted. |
There was a problem hiding this comment.
This timeframe of 1 day is oftentimes not met, due to e.g. other fix PRs taking longer to review, CIs having issues, and similar.
There was a problem hiding this comment.
Thats a failure on the team in my opinion. Reviewing and code velocity are deplorably slow (in my opinion, you are one of the only people doing it right).
There was a problem hiding this comment.
i would probably word this around that with different scopes of validation we might be getting new failures that are not discovered within PR scopes. As larger scopes have different schedules it might happen that problems would be reported hours or even days after PR integration. So i would say 1 day is a good target that we could use from point of issue discovery. In case we understand that 1 day(24 hours) fix is not feasible we should discuss next steps and timelines. Ideally reverting change would be preferable solution, but there might be cases then this would be challenging - such cases should be discussed on exception bases.
Also i would put expectation that PR owner have responsibility for fixing unintentionally caused failures.
Another thing - not all failures would be equally critical - for example while CI/nightly failures are critical i would argue that coverity fixes should be recovered within day - probably longer timeframe is acceptable there.
| bug/issue. | ||
| 3. If the bug fix is associated with an issue raised on Github, it must be | ||
| linked. | ||
| 4. If the bug fix solves the Github issue and is merged, the Github issue |
There was a problem hiding this comment.
I thought we had a policy of closing only after the fix reaches a release.
There was a problem hiding this comment.
I think the rule is more generalized - bug should be closed whenever fix could be used. Some stuff get fixed with commit itself, while other changes would require release.
Also it make sense to keep bug open so we will not be getting multiple submissions on same problem.
|
|
||
| ## Section 5: Documentation | ||
|
|
||
| 1. The PR title must include the related feature and/or DAAL/oneDAL component. |
There was a problem hiding this comment.
Oftentimes they are general, like changes in the shell scripts.
There was a problem hiding this comment.
This applies to changes with label "documentation".
There was a problem hiding this comment.
Yes, and the docs are built through shell scripts.
|
|
||
| ## Section 10: API/ABI changes | ||
|
|
||
| 1. Changes to the API/ABI should be specifically scheduled for merging at |
There was a problem hiding this comment.
Could we start using version macros for this instead? It'd require additional testing infrastructure though.
There was a problem hiding this comment.
Thoughts on how we do API/ABI changes @napetrov @syakov-intel ?
There was a problem hiding this comment.
@david-cortes-intel could you elaborate on version macros?
There was a problem hiding this comment.
overall i would rephrase this as API/ABI breaking changes are only possible with major version increments and thus should be planned for such version release.
There was a problem hiding this comment.
In some cases, it should be possible to use #ifdef with the oneDAL version as part of the macro parameters to make it have different code conditioned on what the version number is at compile time.
|
|
||
| ### Notes: | ||
|
|
||
| \* Comment with text '/intelci: run' on the Github PR will automatically run |
There was a problem hiding this comment.
Does this also applied to comments posted by external contributors?
There was a problem hiding this comment.
This is related to note on how to run private CI, should be useful for external contributors on how to trigger it.
There was a problem hiding this comment.
@icfaust Does the internal CI trigger if a random user (who is not in the intel/uxl orgs) posts the comment?
There was a problem hiding this comment.
I think how to run CI section should be moved out of proposal and referenced instead. it would be modified more frequently than PR rules themself.
There was a problem hiding this comment.
@icfaust Does the internal CI trigger if a random user (who is not in the intel/uxl orgs) posts the comment?
no, it would be started only by folks working at Intel
| \*\*\*\*\*\* direct links to the run should be modified to refer to | ||
| http://intel-ci.intel.com/ and not internal Intel servers. | ||
|
|
||
| # end of text |
There was a problem hiding this comment.
I think this line can be left out.
There was a problem hiding this comment.
As the text of the documentation is embedded in the RFC, I needed to signify when the rules are complete, and the rest of the RFC (as per the RFC rules) can be included, it will be dropped when added to the documentation.
| Generating, reviewing and merging code in oneDAL are key pillars in providing | ||
| a high quality and performant product. The oneDAL codebase is both large and | ||
| intricate requiring domain knowledge and precision. In order to guarantee these | ||
| qualities in oneDAL for all contributors, a set of rules should be added. |
There was a problem hiding this comment.
| qualities in oneDAL for all contributors, a set of rules should be added. | |
| qualities in oneDAL for all contributors, a set of rules should be added. |
| qualities in oneDAL for all contributors, a set of rules should be added. | |
| qualities in oneDAL for all contributors, a set of rules is defined for how to generate, review and merge pull requests. |
|
|
||
| ## Proposal | ||
|
|
||
| This enumerates in detail the aspects and proceedures necessary to getting a |
There was a problem hiding this comment.
| This enumerates in detail the aspects and proceedures necessary to getting a | |
| This enumerates in detail the aspects and proceedures necessary to getting a |
| This enumerates in detail the aspects and proceedures necessary to getting a | |
| This enumerates in detail the aspects and procedures necessary to getting a |
Run a spell checker over this. If you are on a unix OS, something like aspell works quite well for text files.
| 3. Marking PRs as Ready for Review | ||
| * Before marking a PR as Ready for Review: | ||
| * All tasks in the checklist must be completed. | ||
| * Features and changes must be fully implemented. |
There was a problem hiding this comment.
I would suggest that something is ready to go in if it either passes existing tests (where changing current functionality), or passes new tests added for new functionality. Maybe wording like the following?
* Where new functionality is added, new tests should also be added to test the functionality. If a situation arises where new code is not tested, or cannot be tested, clear justification must be given in the PR as to why this is the case.
I think that follows on from the next bullet point about CI passing, and should come after it.
With those changes to the wording, that satisfies as what I see as a 'fully implemented' change. One that does not break existing functionality, and that proves (apart from exceptional cases) that new code works.
| 12. During a code-freeze period (before drop), PRs should only be pulled in by | ||
| main maintainers. | ||
| 13. An approval should be given with the expectation to merge, use best | ||
| judgement in reviewing and approving PRs (get help as when not confident). |
There was a problem hiding this comment.
| judgement in reviewing and approving PRs (get help as when not confident). | |
| judgement in reviewing and approving PRs (get help as when not confident). |
| judgement in reviewing and approving PRs (get help as when not confident). | |
| judgement in reviewing and approving PRs. When not confident, help should be requested from a maintainer or appropriate CODEOWNER. |
|
|
||
| ## Preamble: | ||
|
|
||
| This is the rules for merging PRs into the oneDAL code base on Github. This |
There was a problem hiding this comment.
As far oneDAL and scikit-learn-intelex have interdependency it would make sense to clarify this aspect in greater details, especially for cases that would be involving modifications on both repos
| ## Section 1: Opening a PR, Ready for Review | ||
|
|
||
| 1. PR Template and Task Checklist | ||
| * All PRs must use the provided template, including the task checklist. |
There was a problem hiding this comment.
Let's mention checklist scope definition - currently it's assume people will be removing not relevant checkboxes.
Might be instead limit them to minimal set, providing option for selecting is some of those are not applicable?
| * Before marking a PR as Ready for Review: | ||
| * All tasks in the checklist must be completed. | ||
| * Features and changes must be fully implemented. | ||
| * Both private and public CI pipelines must pass, or clear justification for any |
There was a problem hiding this comment.
i would add large disclaimer here - as scopes of CI's are different and changing overtime. So currently yes things depend on internal Intel CI, but lot of stuff already enabled in public. Ideally we should be able to track this separately so no changes to overall PR rules would be required
| the number of issues for the Intel internal code scans\*\*\*, and may be | ||
| grounds for a reversion. | ||
| 5. A merged PR which causes a code scan failure should be fixed before the next |
There was a problem hiding this comment.
I wouldn't be focusing on Intel internal scans and would write this as generic code scans. Some of them are public, while others not yet.
I think from internal scans at this point we have Coverity for oneDAL - which will be migrated to public instance.
Second one is protex IP - this one is generally usefull as it would catch potential not attributed contributions and license conflicts. I'm not sure if we have open alternatives for this.
| grounds for a reversion. | ||
| 5. A merged PR which causes a code scan failure should be fixed before the next | ||
| scheduled code scan can be reverted. | ||
| 6. A designated person is assigned (for a determined period of time), who |
There was a problem hiding this comment.
Sorry - don't get how this works, what is intent here?
| scheduled code scan can be reverted. | ||
| 6. A designated person is assigned (for a determined period of time), who | ||
| determines which PR was responsible for the failure. | ||
| 7. All PRs must have a label describing the purpose. |
There was a problem hiding this comment.
should we consider auto labeling besides of the requirement itself?
| * Changes to public facing header files etc. | ||
| * Changes to build structure (e.g. makefiles, bazel. etc.) | ||
| 11. Commits should follow commit message rules. | ||
| 12. During a code-freeze period (before drop), PRs should only be pulled in by |
There was a problem hiding this comment.
i think there should be no code-freeze period and release branches should be created instead straight away.
Also there is no drop in terms of public releases. It might be better to just talk in form of release branches being restricted with commits there + we already have this implemented with github permissions
| * Changes to build structure (e.g. makefiles, bazel. etc.) | ||
| 11. Commits should follow commit message rules. | ||
| 12. During a code-freeze period (before drop), PRs should only be pulled in by | ||
| main maintainers. |
There was a problem hiding this comment.
we could replace this with release management group - https://github.com/uxlfoundation/oneDAL/blob/main/MAINTAINERS.md#release-management
| main maintainers. | ||
| 13. An approval should be given with the expectation to merge, use best | ||
| judgement in reviewing and approving PRs (get help as when not confident). | ||
| 14. The submitter is responsible for the maintenance (including closing) of |
There was a problem hiding this comment.
i would put this broader as submitter is responsible for pushing PR towards integration
| the author is responsible resolving with reviewer the follow up steps in | ||
| detail. | ||
| 20. Changes to these rules must be proposed following the RFC process. | ||
| 21. Additional private CI runs can be requested. |
There was a problem hiding this comment.
would generalize this - additional validation/measurements
| detail. | ||
| 20. Changes to these rules must be proposed following the RFC process. | ||
| 21. Additional private CI runs can be requested. | ||
| 22. While oneDAL is open source, hardware IP should be protected especially in |
There was a problem hiding this comment.
I think we could remove this - those are not generic expectation on oneDAL contributors but corporations specific expectations that are different for different companies
| 23. An admin can wipe a review or otherwise change a PR as necessary. (Reviewer | ||
| unavailable to re-review, etc.) | ||
| 24. Description must describe the change and reasoning behind it. | ||
| 25. These are not hard and fast and can be changed in cases that warrant it. |
There was a problem hiding this comment.
Looks it cut out from broader context
| unavailable to re-review, etc.) | ||
| 24. Description must describe the change and reasoning behind it. | ||
| 25. These are not hard and fast and can be changed in cases that warrant it. | ||
| 26. When merging, use the title of the PR and not of the commit(s). |
There was a problem hiding this comment.
can we check/validate this?
| release) or require substantial time to implement. | ||
| * Other blockers prevent the changes from being merged into the current main branch. | ||
|
|
||
| ## Section 2: General Rules |
There was a problem hiding this comment.
would also add line one code ownership - new code contributions shouldn't violate license rights nether on source where they are taken, nor oneDAL license.
Code reuse with appropriate licensing is possible but proper references should be made for in code including original copyrights/attributions
|
|
||
| ## Section 8: Performance | ||
|
|
||
| 1. Performance benchmarks must include scikit-learn_bench\*\*\*\*\* if |
There was a problem hiding this comment.
i would generalize this as appropriate benchmark results. Not everything could be covered with scikit-learn_bench.
|
|
||
| 1. Performance benchmarks must include scikit-learn_bench\*\*\*\*\* if | ||
| the algorithm is included or to be included in scikit-learn-intelex. | ||
| 2. Performance benchmarks must be provided to reviewers via email. |
There was a problem hiding this comment.
Is this unnecessary ? i think any channel would work, even if results would be shared within PR itself
| 1. Performance benchmarks must include scikit-learn_bench\*\*\*\*\* if | ||
| the algorithm is included or to be included in scikit-learn-intelex. | ||
| 2. Performance benchmarks must be provided to reviewers via email. | ||
| 3. Code with SYCL-specific implementations should include specific benchmarking |
There was a problem hiding this comment.
i think this should be generalized that based on changes corresponding HW platforms should be chosen.
| 3. Ideally, the PR should refer to the feature introduction/or latest change | ||
| to feature PR. | ||
|
|
||
| ## Section 10: API/ABI changes |
There was a problem hiding this comment.
Potentially it worth adding note on declaring deprecations - those can be done at any time and might be we need entire section going through details there
| independently find and understand CI regressions via some medium outside the PR | ||
| itself (for example confluence, email chain, etc.) | ||
|
|
||
| \*\*\* Some code scans can be found in the DAAL CI_DAAL Run, some may only run |
There was a problem hiding this comment.
This is not clear what "DAAL CI_DAAL Run" mean here
| \*\*\*\*\* scikit-learn_bench report structure is the standard, but other runs | ||
| can be used at reviewers discretion. | ||
|
|
||
| \*\*\*\*\*\* direct links to the run should be modified to refer to |
There was a problem hiding this comment.
this is just internal expectation, not generic one
|
@icfaust thanks a lot for using RFCs One thing to add - point to coding/contribution guidelines. i think it wasn't explicitly mentioned that contribution should be aligned with them |
4 participants
Description
This is a modified version of internal rules used for oneDAL Pull Requests. These are to be made public with necessary modification representing the facts-on-the-ground of recent oneDAL development. Discussion of this should occur before
integration into oneDAL's documentation. This should prove useful for users as to the steps and proceedures behind labeling,
the tasklists, and behavior of developer and reviewer.
This should help to standardize and speed development in oneDAL.
Rendered RFC can be found here: https://github.com/icfaust/oneDAL/tree/rfcs/PR-rules/rfcs/20250618-pr-rules
PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.
You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance