-
Notifications
You must be signed in to change notification settings - Fork 184
DOC, MAINT: Centralize and expand development-related docs #2734
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: main
Are you sure you want to change the base?
DOC, MAINT: Centralize and expand development-related docs #2734
Conversation
| .. hint:: If building the library in-place without installing, it's then necessary to set environment variable ``$PYTHONPATH`` to point to the root of the repository in order to ble able to import the modules in Python. | ||
|
|
||
| Build using conda |
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.
@Alexsandruss Please review this part thoroughly and feel free to modify if it's unclear or if it's missing something.
| Topics for Contributors | ||
| ======================= | ||
|
|
||
| Adding an estimator |
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 Please review this section in particular.
| else: | ||
| # code branch for sklearn<1.7 | ||
| Test helpers |
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 I might be missing something here so please review thoroughly.
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| .. include:: substitutions.rst | ||
|
|
||
| ======================= | ||
| Topics for 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.
@icfaust Feel free to add some instructions about what would be needed in order to add a patched function that is not an estimator class, like a kernel or 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.
Pull Request Overview
This PR consolidates development-related documentation from scattered markdown and reStructuredText files into centralized Sphinx documentation under the doc/sources/ directory, improving organization and maintainability. The changes assume a "development" deployment of documentation will be available at https://uxlfoundation.github.io/scikit-learn-intelex/development.
Key changes:
- Migrates installation, testing, and contribution guidelines from standalone
.mdfiles to.rstdocumentation files - Updates internal and external links to point to the new documentation URLs
- Removes redundant standalone files (
INSTALL.md,CONTRIBUTING.md,SUPPORT.md,scikit-learn-tests.md) - Reorganizes documentation structure with new "Development guides" section
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/util_compare_json_reports.py | Updated comment link to point to new documentation URL |
| scikit-learn-tests.md | Removed file (content moved to doc/sources/tests.rst) |
| doc/sources/topics-for-contributors.rst | New comprehensive guide for contributors covering estimators, versioning, tests, benchmarks, and docs |
| doc/sources/tests.rst | New consolidated testing documentation including test execution and scikit-learn conformance tests |
| doc/sources/support.rst | Expanded support documentation with additional contact methods |
| doc/sources/quick-start.rst | Updated link to building-from-source documentation |
| doc/sources/index.rst | Reorganized with new "Development guides" section and updated navigation structure |
| doc/sources/ideas.rst | Updated title for clarity |
| doc/sources/distributed_daal4py.rst | Updated link to building-from-source documentation |
| doc/sources/distributed-mode.rst | Updated link to building-from-source documentation |
| doc/sources/code-of-conduct.rst | Converted from markdown to reStructuredText format |
| doc/sources/building-from-source.rst | New comprehensive guide for building from source (consolidates INSTALL.md content) |
| doc/sources/algorithms.rst | Updated scikit-learn test suite reference to use internal link |
| daal4py/README.md | Simplified to redirect to documentation page |
| SUPPORT.md | Removed file (content moved to doc/sources/support.rst) |
| README.md | Updated links to point to documentation pages instead of markdown files |
| INSTALL.md | Removed file (content moved to doc/sources/building-from-source.rst) |
| CONTRIBUTING.md | Removed file (content moved to documentation) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Vika-F
left a 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.
Great contribution! It contains a lot of information useful for the contributors.
I found only one typo, please fix before merge. Also added Copilot to check for other typos, if any.
And there is a couple of questions you might want to address.
doc/sources/building-from-source.rst
Outdated
| rm -Rf build | ||
| TBB runtimes |
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 it be oneTBB?
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.
Renamed to OneTBB.
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 Thank you, but I think it should start with a lower case 'o'. Same as 'oneDAL'.
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.
Renamed.
| - ``NO_DIST``: set to '1', 'yes' or alike to build without support for distributed mode. | ||
| - ``NO_STREAM``: set to '1', 'yes' or alike to build without support for streaming mode. | ||
| - ``NO_DPC``: set to '1', 'yes' or alike to build without support of oneDAL DPC++ interfaces. | ||
| - ``OFF_ONEDAL_IFACE``: set to '1' to build without the support of oneDAL interfaces. |
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 it make sense at all?
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 don't think so given that there's also build_ext, but that's what the script is designed to recognize.
Co-authored-by: Victoriya Fedotova <viktoria.nn@gmail.com>
Description
Currently, documentation is spread throughout a mixture of .md and .rst files in different places. This PR centralizes all of the development-related docs into a single place, and updates links accordingly.
Along the way, it also adds more links and details about some topics that the current docs are missing.
IMPORTANT!! In order for this PR to work without breaking anything until a subsequent release, it will be required to have some kind of job uploading doc pages from the main branch - e.g. as a "development" version in the version switcher. This PR assumes that there will be a deployment available under that name, so it adds the currently non-existent links to it in the readme:
https://uxlfoundation.github.io/scikit-learn-intelex/development
Note however that the PR doesn't touch the file
SECURITY.md, since it appears to be a hard-coded name for the OSSF scorecard and for some github built-in features.Checklist:
Completeness and readability
Testing