Skip to content
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

Version support and automatic CI build #2214

Closed
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Adds version CI and version support
Signed-off-by: Putnam, Kevin <[email protected]>
intelkevinputnam committed Dec 6, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit b6bd557021cf62a66b2ad629cdcfeeaf5800e3ec
47 changes: 47 additions & 0 deletions .github/workflows/publish.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
name: publish
'on':
push:
branches:
docs-ci

jobs:
build:

runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v3
- name: install dependencies
run: |
sudo apt update
sudo apt install -y pandoc git
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
sudo apt install -y pandoc git
sudo apt install -y pandoc

How come git is necessary?

pip3 install -r requirements-doc.txt
pip3 install scikit-learn-intelex
Copy link
Contributor

Choose a reason for hiding this comment

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

I had forgotten to comment here but: we'd probably want to change this logic towards either building from source, or downloading artifacts from github - otherwise this wouldn't work out when pushing to release branches (would build docs for the wrong version), nor when trying to build docs for unreleased versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally agree.

cd doc
source set_version.sh
echo Generating version $DOC_VERSION
Comment on lines +20 to +22
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
cd doc
source set_version.sh
echo Generating version $DOC_VERSION
DOC_VERSION=$(python -m pip freeze | grep -oP "(?<=scikit-learn-intelex==)\d*\.\d*")
echo Generating version $DOC_VERSION
echo "DOC_VERSION=${DOC_VERSION}" >> "$GITHUB_ENV"

Add it to the github environment to save some work/lines going forward.

- name: build docs
run: |
cd doc
chmod +x build-doc.sh
./build-doc.sh
mv _build/scikit-learn-intelex ../../output
Comment on lines +25 to +28
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
cd doc
chmod +x build-doc.sh
./build-doc.sh
mv _build/scikit-learn-intelex ../../output
doc/build-doc.sh
mv doc/_build/scikit-learn-intelex ../output

Sorry since I am not so knowledgeable on the docs side. If build-doc.sh doesn't have the right permissions, we can set those in the repo to what is necessary (does it hurt to store build-doc.sh as 755 instead)?

- name: deploy docs
run: |
cd doc
source set_version.sh
cd ..
Comment on lines +31 to +33
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
cd doc
source set_version.sh
cd ..

With the previous suggestion these 3 lines are unnecessary as DOC_VERSION will be propagated in the environment between steps.

git fetch
git checkout -- doc/build-doc.sh
git checkout gh-pages
rm -rf $DOC_VERSION
mv -f ../output/$DOC_VERSION .
mv ../output/versions.json .
mv ../output/index.html .
rm -rf doc
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 rm wipe the git checkout of the build-doc.sh?

git config --global user.name "${GITHUB_ACTOR}"
git config --global user.email "${GITHUB_ACTOR}@github.com"
git add .
git commit -m "latest HTML output"
git push -f https://${GITHUB_ACTOR}:${{secrets.GITHUB_TOKEN}}@github.com/${GITHUB_REPOSITORY}.git HEAD:gh-pages
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: what happens if something goes wrong during the build? For example, we might run into errors such as failing to import the modules from which autodoc generates documentation, which would manifest as a "warning" being issued but some docs still being built.

Could there be a check for build warnings that would make this script fail (avoiding pushing anything to GH) if something goes wrong?

Copy link
Author

Choose a reason for hiding this comment

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

Any errors in the github workflow will stop the docs from publishing. We could turn on "error on warning" in the sphinx build, if we wanted to gate against any issue in the Sphinx build.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that'd be ideal, so that we don't accidentally end up with empty docs. But would be better if it could ignore some warnings selectively - currently there are some warnings when building daal4py which I'm not sure how to solve and which do not impact the resulting docs that get built.

Copy link
Contributor

Choose a reason for hiding this comment

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

Example benign warnings when building daal4py:

/home/dcortes/repos/scikit-learn-intelex/doc/daal4py/docstring of daal4py._daal4py.classifier_prediction_result:1: WARNING: duplicate object description of daal4py.classifier_prediction_result, other instance in algorithms, use :noindex: for one of them

Copy link
Contributor

Choose a reason for hiding this comment

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

https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication#example-1-passing-the-github_token-as-an-input let me know if i am missing something, sentence just before this link: "Commits pushed by a GitHub Actions workflow that uses the GITHUB_TOKEN do not trigger a GitHub Pages build."


11 changes: 9 additions & 2 deletions doc/build-doc.sh
Original file line number Diff line number Diff line change
@@ -27,6 +27,13 @@ mkdir $SAMPLES_DIR
cd ..
rsync -a --exclude='doc/$SAMPLES_DIR/daal4py_data_science.ipynb' examples/notebooks/*.ipynb doc/$SAMPLES_DIR

# build the documentation
cd doc
make html

source ./set_version.sh
export SPHINXPROJ=scikit-learn-intelex
Copy link
Contributor

Choose a reason for hiding this comment

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

We also have daal4py with docs under doc/daal4py.

export BUILDDIR=_build
export SOURCEDIR=sources

sphinx-build -b html $SOURCEDIR $BUILDDIR/$SPHINXPROJ/$DOC_VERSION
cp versions.json $BUILDDIR/$SPHINXPROJ
echo "<meta http-equiv=\"refresh\" content=\"0; URL='/$SPHINXPROJ/$DOC_VERSION/'\" / >" >> $BUILDDIR/$SPHINXPROJ/index.html
15 changes: 15 additions & 0 deletions doc/get_doc_version.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import subprocess
import os

def get_version():
Copy link
Contributor

Choose a reason for hiding this comment

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

@icfaust Could you please look into whether this captures the version number correctly.

Copy link
Contributor

@icfaust icfaust Jan 15, 2025

Choose a reason for hiding this comment

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

i would prefer if we got rid of this file and set_version.sh entirely and replaced it with

DOC_VERSION=$(python -m pip freeze | grep -oP "(?<=scikit-learn-intelex==)\d*\.\d*")

major = ''
minor = ''
pip_output = subprocess.run(["pip","list"],capture_output=True,encoding='utf-8')
lines = pip_output.stdout.split("\n")
for line in lines:
if line.startswith("scikit-learn-intelex"):
version = line.split()[1].split(".")
major = version[0]
minor = version[1]

return major + "." + minor
2 changes: 2 additions & 0 deletions doc/set_version.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export DOC_VERSION=$(python -c "from get_doc_version import get_version; print(get_version())")

6 changes: 6 additions & 0 deletions doc/sources/_templates/layout.html
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@
{% block extrahead %}
<script defer type="text/javascript" src="https://www.intel.com/content/dam/www/global/wap/performance-config.js" ></script>
<script type="text/javascript">
fetch("{{ switcher_url }}").then(response => response.text()).then(respText=> load_versions(respText));
// Configure TMS settings
var wapLocalCode = 'us-en'; // Dynamically set per localized site, see mapping table for values
var wapSection = "scikit-learn"; // WAP team will give you a unique section for your site
@@ -15,3 +16,8 @@
}
</script>
{% endblock %}

{% block menu %}
{% include "versions.html" %}
{{ super() }}
{% endblock %}
23 changes: 23 additions & 0 deletions doc/sources/_templates/versions.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<div class="version-switcher__container dropdown">
<script>
current_version = "{{current_version}}"
project_name = "{{project_name}}"
</script>
<div id="version-switcher-button"
class="version-switcher__button"
data-bs-toggle="dropdown"
aria-haspopup="listbox"
aria-controls="version-switcher-dropdown"
aria-label="Version switcher list"
style="display:inline-block;padding-left:10px;padding-right:10px;"
>
Choose version <!-- this text may get changed later by javascript -->
<span class="caret"></span>
</div>
<div id="version-switcher-dropdown"
class="version-switcher__menu dropdown-menu list-group-flush py-0"
role="listbox" aria-labelledby="{{ button_id }}">
<!-- dropdown will be populated by javascript on page load -->
</div>
</div>

17 changes: 14 additions & 3 deletions doc/sources/conf.py
Original file line number Diff line number Diff line change
@@ -23,8 +23,12 @@
# full list see the documentation:
# http://www.sphinx-doc.org/en/master/config

bench_test = False

# -- Path setup --------------------------------------------------------------

import json
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this import is not used.


# If extensions (or modules to document with autodoc) are in another directory,
# add these directories to sys.path here. If the directory is relative to the
# documentation root, use os.path.abspath to make it absolute, like shown here.
@@ -34,17 +38,16 @@

sys.path.insert(0, os.path.abspath("../"))


# -- Project information -----------------------------------------------------

project = "Intel(R) Extension for Scikit-learn*"
copyright = "Intel"
author = "Intel"

# The short X.Y version
version = "2025.0.0"
version = os.environ.get('DOC_VERSION','')
Copy link
Contributor

Choose a reason for hiding this comment

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

How about changing the version to "latest" here when it doesn't find the env. variable?

Copy link
Author

Choose a reason for hiding this comment

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

If there isn't a version, the build should fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean: would be ideal to have a version "latest" building the docs directly from the master branch, for unreleased versions. So in that case, the release versions would have the env. variable whereas the master branch wouldn't.

# The full version, including alpha/beta/rc tags
release = "2025.0.0"
release = version


# -- General configuration ---------------------------------------------------
@@ -154,6 +157,13 @@
"titles_only": False,
}

switcher_url = "/scikit-learn-intelex/versions.json"
if bench_test:
switcher_url = "/versions.json"

html_context = {"current_version": version,
"project_name": "scikit-learn-intelex",
"switcher_url": switcher_url}

# Add any paths that contain custom static files (such as style sheets) here,
# relative to this directory. They are copied after the builtin static files,
@@ -163,6 +173,7 @@

def setup(app):
app.add_css_file("custom.css")
app.add_js_file("version_switcher.js")


# -- Options for HTMLHelp output ---------------------------------------------
19 changes: 19 additions & 0 deletions doc/versions.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
[
Copy link
Contributor

Choose a reason for hiding this comment

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

What about "stable" version tag in addition to latest?

Copy link
Author

Choose a reason for hiding this comment

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

Since this file needs to be manually curated, we can add any tags we want to it.

{
"name": "2025.0 (latest)",
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 make this file auto-generated from the releases that we have as github tags instead? It's BTW missing some version numbers, such as 2024.7.0:
https://github.com/uxlfoundation/scikit-learn-intelex/releases

Copy link
Author

Choose a reason for hiding this comment

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

This file will need to be manually curated, since it is the list of versions that populates the switcher for all published versions. You'll need to remove old versions and stipulate which one is latest. The build script shouldn't know which is latest, since you could possible publish an older version (that isn't latest)

Copy link
Contributor

Choose a reason for hiding this comment

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

The list of releases can be obtained programmatically. This repository keeps branches with "rls" in the name for all released versions, which can be obtained with a command like this from the root of the repository:

git branch -r | grep rls

And since it follows date-based versioning, the most up-to-date one will always be the last one in alphabetical order, as returned by that command. So it should be possible to auto-generate this file with all the releases that exist, and keep it auto-updated to point to the last when a new branch comes in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although the issue would be that some versions are missing docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have to agree with @david-cortes-intel on this one, we could technically have the (latest) tag be updated on a github create trigger when it has rls in it.

"version": "2025.0",
"url": "/scikit-learn-intelex/2025.0/"
},
{
"version": "2024.6",
"url": "/scikit-learn-intelex/2024.6/"
},
{
"version": "2024.3",
"url": "/scikit-learn-intelex/2024.3/"
},
{
"version": "2023.2",
"url": "/scikit-learn-intelex/2023.2/"
}
]