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

fix: trigger doc build all condition #2242

Open
wants to merge 96 commits into
base: main
Choose a base branch
from

Conversation

yuejiaointel
Copy link
Contributor

@yuejiaointel yuejiaointel commented Jan 6, 2025

Description

in this PR I fixed several problems so now doc can build without autodoc warnings, also I added code so in future if autodoc gives a warning it will be marked as an error so pipeline will fail. It still has duplicate warnings because we are having multiple same classes def in daal4py.

_List associated issue number(s) if exist(s): #8786

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.

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Flag Coverage Δ
azure 78.04% <ø> (+1.25%) ⬆️
github 71.06% <ø> (+0.86%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 17 files with indirect coverage changes

@david-cortes-intel
Copy link
Contributor

Would the problem that this PR addresses get fixed as part of this other PR?
#2214

@yuejiaointel
Copy link
Contributor Author

Would the problem that this PR addresses get fixed as part of this other PR? #2214

Hi David,
I think it is different, the ref pr seems about making sure the doc are generated with the right version, but this one is mainly about fix the warning about doc, making sure doc can be generated completely, and bring back doc in ci workflow, now it is skipped.
Best,
Yue

.ci/pipeline/docs.yml Show resolved Hide resolved
.ci/pipeline/docs.yml Outdated Show resolved Hide resolved
.ci/pipeline/docs.yml Outdated Show resolved Hide resolved
.ci/pipeline/docs.yml Outdated Show resolved Hide resolved
@yuejiaointel
Copy link
Contributor Author

yuejiaointel commented Jan 22, 2025

@yuejiaointel Looks like something went wrong, despite the CI status and the warning failure logic: image

Hi David,
I did some experiments on this problem and I think using

python setup.py build_ext --inplace --abs-rpath
python setup.py build --abs-rpath

does not work. First of all, python setup.py build_ext --inplace --abs-rpath
will still copy the _daal4py.cpython*.so back into ./daal4py, and then in later sklearnex documentation build it will interpret the daal4py folder in doc as the folder to import daal4py. As a result, the sklearnex doc is not build because it could not import the modules. I think the reason is using the --inplace pritorize local dir more than the site package and we have a daal4py in root project and another in doc and it is prioritizing local daal4py folder depending where we are that is why we see different behavior in different folders.

Experiment result I did:
python -c "import daal4py; print('Using daal4py from:', daal4py.file)"
Before entering doc daal4py.file is referring to daal4py in root
after entering doc daal4py.file is None

But using:

./conda-recipe/build.sh
cp ${DALROOT}/lib/python3.11/site-packages/daal4py/_daal4py.cpython*.so daal4py/

daal4py.file is referring site-packages daal4py which I think is more ideal
What I am still not understanding is why we need to cp the *.so file back to daal4py folder in root project even when we can referring daal4py in the site packages.

@david-cortes-intel
Copy link
Contributor

@yuejiaointel Looks like something went wrong, despite the CI status and the warning failure logic: image

Hi David, I did some experiments on this problem and I think using

python setup.py build_ext --inplace --abs-rpath python setup.py build --abs-rpath

does not work. First of all, python setup.py build_ext --inplace --abs-rpath will still copy the _daal4py.cpython*.so back into ./daal4py, and then in later sklearnex documentation build it will interpret the daal4py folder in doc as the folder to import daal4py. As a result, the sklearnex doc is not build because it could not import the modules. I think the reason is using the --inplace pritorize local dir more than the site package and we have a daal4py in root project and another in doc and it is prioritizing local daal4py folder depending where we are that is why we see different behavior in different folders.

Experiment result I did: python -c "import daal4py; print('Using daal4py from:', daal4py.file)" Before entering doc daal4py.file is referring to daal4py in root after entering doc daal4py.file is None

But using:

./conda-recipe/build.sh cp ${DALROOT}/lib/python3.11/site-packages/daal4py/_daal4py.cpython*.so daal4py/

daal4py.file is referring site-packages daal4py which I think is more ideal What I am still not understanding is why we need to cp the *.so file back to daal4py folder in root project even when we can referring daal4py in the site packages.

It is not required to manually copy the file.

If you use the build script, it will install the modules into site-packages (they are directly importable), whereas if you use setup.py build, it will build it in-place in the root folder, so attempting to import them directly requires either manually adding them to ${PYTHONPATH} (or to sys.path), or launching the python interpreter from the folder where they are (the root of the repository, not /doc).

I guess the issue could be that the root of the docs folder might be in the python's path and thus import daal4py makes it try to import the module from the root folder instead of site packages. There's also these lines here which could be having an effect:

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

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

See this example which is working with in-place builds:
https://github.com/uxlfoundation/scikit-learn-intelex/pull/2269/files#diff-3f4a99ddd4f584a56e293b58a9a1a9c0d514c381b336d3f1ceaa7b2fcc55a902L78

@yuejiaointel
Copy link
Contributor Author

@yuejiaointel Looks like something went wrong, despite the CI status and the warning failure logic: image

Hi David, I did some experiments on this problem and I think using
python setup.py build_ext --inplace --abs-rpath python setup.py build --abs-rpath
does not work. First of all, python setup.py build_ext --inplace --abs-rpath will still copy the _daal4py.cpython*.so back into ./daal4py, and then in later sklearnex documentation build it will interpret the daal4py folder in doc as the folder to import daal4py. As a result, the sklearnex doc is not build because it could not import the modules. I think the reason is using the --inplace pritorize local dir more than the site package and we have a daal4py in root project and another in doc and it is prioritizing local daal4py folder depending where we are that is why we see different behavior in different folders.
Experiment result I did: python -c "import daal4py; print('Using daal4py from:', daal4py.file)" Before entering doc daal4py.file is referring to daal4py in root after entering doc daal4py.file is None
But using:
./conda-recipe/build.sh cp ${DALROOT}/lib/python3.11/site-packages/daal4py/_daal4py.cpython*.so daal4py/
daal4py.file is referring site-packages daal4py which I think is more ideal What I am still not understanding is why we need to cp the *.so file back to daal4py folder in root project even when we can referring daal4py in the site packages.

It is not required to manually copy the file.

If you use the build script, it will install the modules into site-packages (they are directly importable), whereas if you use setup.py build, it will build it in-place in the root folder, so attempting to import them directly requires either manually adding them to ${PYTHONPATH} (or to sys.path), or launching the python interpreter from the folder where they are (the root of the repository, not /doc).

I guess the issue could be that the root of the docs folder might be in the python's path and thus import daal4py makes it try to import the module from the root folder instead of site packages. There's also these lines here which could be having an effect:

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

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

See this example which is working with in-place builds: https://github.com/uxlfoundation/scikit-learn-intelex/pull/2269/files#diff-3f4a99ddd4f584a56e293b58a9a1a9c0d514c381b336d3f1ceaa7b2fcc55a902L78

Hi David!
Thx! This make senses now. I commented out the lines in both conf and removed the cp command. Now I just need one line of running build.sh and it works
Best,
Yue

Copy link
Contributor

@david-cortes-intel david-cortes-intel left a comment

Choose a reason for hiding this comment

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

LGTM, some small comments.

@@ -33,4 +33,4 @@ help:
# Catch-all target: route all unknown targets to Sphinx using the new
# "make mode" option. $(O) is meant as a shortcut for $(SPHINXOPTS).
%: Makefile
@$(SPHINXBUILD) -M $@ "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)
@$(SPHINXBUILD) -M $@ "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O)
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 file is not being modified. Please reset it to how it has before the PR so as not to mess the history.

@@ -17,8 +17,7 @@
trigger:
branches:
include:
- main
- rls/*
- "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remember to change this before merging.

@david-cortes-intel
Copy link
Contributor

@yuejiaointel Before merging, could you do an experiment here checking that the CI job would show in red if it fails to import daal4py?

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

Successfully merging this pull request may close these issues.

3 participants