Skip to content

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

Draft
wants to merge 17 commits into
base: rfcs
Choose a base branch
from

Conversation

icfaust
Copy link
Contributor

@icfaust icfaust commented Jun 18, 2025

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

  • I have reviewed my changes thoroughly before submitting this pull request.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes or created a separate PR with update and provided its number in the description, if necessary.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have added a respective label(s) to PR if I have a permission for that.
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • I have run it locally and tested the changes extensively.
  • All CI jobs are green or I have provided justification why they aren't.
  • I have extended testing suite if new functionality was introduced in this PR.

Performance

  • I have measured performance for affected algorithms using scikit-learn_bench and provided at least summary table with measured data, if performance change is expected.
  • I have provided justification why performance has changed or why changes are not expected.
  • I have provided justification why quality metrics have changed or why changes are not expected.
  • I have extended benchmarking suite and provided corresponding scikit-learn_bench PR if new measurable functionality was introduced in this PR.

@icfaust icfaust added the RFC label Jun 18, 2025
@david-cortes-intel
Copy link
Contributor

@icfaust Text has redundant spaces after each period.


## Preamble:

This is the rules for merging PRs into the oneDAL code base on Github. This
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Contributor

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.

Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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).

Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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".

Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

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 ?

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.
Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This enumerates in detail the aspects and proceedures necessary to getting a
This enumerates in detail the aspects and proceedures necessary to getting a
Suggested change
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.
Copy link
Contributor

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).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
judgement in reviewing and approving PRs (get help as when not confident).
judgement in reviewing and approving PRs (get help as when not confident).
Suggested change
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
Copy link
Contributor

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.
Copy link
Contributor

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?

conda-forge/intel_repack-feedstock#98

* 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
Copy link
Contributor

@napetrov napetrov Jun 18, 2025

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

Comment on lines +57 to +59
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
Copy link
Contributor

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
Copy link
Contributor

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.
Copy link
Contributor

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
Copy link
Contributor

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.
Copy link
Contributor

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
Copy link
Contributor

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.
Copy link
Contributor

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
Copy link
Contributor

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.
Copy link
Contributor

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).
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@napetrov
Copy link
Contributor

@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

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

Successfully merging this pull request may close these issues.

4 participants