-
Notifications
You must be signed in to change notification settings - Fork 224
RFC: Add Pull Request rules to oneDAL #3264
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
base: rfcs
Are you sure you want to change the base?
Conversation
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be skipped in many cases, like changes to readme files and docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we had a policy of closing only after the fix reaches a release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts @napetrov @syakov-intel ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oftentimes they are general, like changes in the shell scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This applies to changes with label "documentation".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we start using version macros for this instead? It'd require additional testing infrastructure though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on how we do API/ABI changes @napetrov @syakov-intel ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-cortes-intel could you elaborate on version macros?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also applied to comments posted by external contributors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line can be left out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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