10000 DOC fix glossary link by lesteve · Pull Request #23534 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

DOC fix glossary link #23534

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jun 6, 2022
Merged

DOC fix glossary link #23534

merged 7 commits into from
Jun 6, 2022

Conversation

lesteve
Copy link
Member
@lesteve lesteve commented Jun 3, 2022 •
edited
Loading

In https://scikit-learn.org/dev/developers/performance.html
The link is http://scikit-learn.org/dev/glossary.html#term-warm-start rather than http://scikit-learn.org/dev/glossary.html#term-warm_start so it does not go to the right place in the glossary.

This allows to double-check #23508 as well.

lesteve
lesteve commented Jun 3, 2022
View reviewed changes
doc/developers/performance.rst
@@ -268,7 +268,7 @@ Then, setup the magics in a manner similar to ``line_profiler``.
- **Under IPython 0.11+**, first create a configuration profile:

.. prompt:: bash $

Copy link
Member Author

Choose a reason for hiding this comment

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

pre-commit made me do it 😉

@lesteve
Copy link
Member Author
lesteve commented Jun 3, 2022 •
edited
Loading

So now the CircleCI workflow is triggered thanks to #23508 but the "check the rendered doc" direct link to the _changed.html is not working yet (there is none "check the rendered doc" CI status for now).

There is a "build-doc-and-deploy" Circle workflows but it encompasses many jobs (doc, doc-min-dependencies, deploy) and the .github/workflows/artifact-redirector.yml expects a job name where you can directly see the artifact.

Not sure about the best approach to improve the situation ... cc @thomasjpfan @alfaro96

Note that through a few clicks you can look at the rendered doc:
build-doc-and-deploy "Details" -> doc build -> artifacts tab -> click "_changed.html"

@lesteve lesteve mentioned this pull request Jun 3, 2022
Merged
@alfaro96
Copy link
Member
alfaro96 commented Jun 3, 2022

So now the CircleCI workflow is triggered thanks to #23508 but the "check the rendered doc" direct link to the _changed.html is not working yet (there is none "check the rendered doc" CI status for now).

There is a "build-doc-and-deploy" Circle workflows but it encompasses many jobs (doc, doc-min-dependencies, deploy) and the .github/workflows/artifact-redirector.yml expects a job name where you can directly see the artifact.

Not sure about the best approach to improve the situation ... cc @thomasjpfan @alfaro96

Note that through a few clicks you can look at the rendered doc:

build-doc-and-deploy "Details" -> doc build -> artifacts tab -> click "_changed.html"

Hi @lesteve, I think we can create a check status without that action. I will work on that.

ogrisel
ogrisel approved these changes Jun 3, 2022
View reviewed changes
Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

+1 for the fix it-self but let's keep this PR open as a simple way to check the new doc-building CI config.

@thomasjpfan
Copy link
Member

.github/workflows/artifact-redirector.yml expects a job name where you can directly see the artifact.

From my understanding artifact-redirector.yml is configured correctly. It is pointing at one of the circleci jobs: doc and once that job finishes the redirector should update the checks.

Maybe there is a hint at: https://github.com/braindecode/braindecode, they have a similar setup that works together with artifact-redirector.yml.

@thomasjpfan
Copy link
Member
thomasjpfan commented Jun 3, 2022 •
edited
Loading

The issue was that I enabled "GitHub Checks" on CircleCI recently. With "GitHub Checks" on it changed the name of the job shown to GitHub to the workflow name. Changing the name makes it incompatible with the circleci-redirector.

I disabled the CircleCI's "GitHub Checks" feature and the circleci-redirector works now.

thomasjpfan
thomasjpfan reviewed Jun 3, 2022
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

@ogrisel
Copy link
Member
ogrisel commented Jun 4, 2022

I disabled the CircleCI's "GitHub Checks" feature and the circleci-redirector works now.

I still cannot see the "check the rendered doc" entry in the last build.

@ogrisel
Copy link
Member
ogrisel commented Jun 4, 2022

Actually I do, it's named: "Check the rendered docs here!"

@ogrisel
Copy link
8000
Member
ogrisel commented Jun 4, 2022

The _changed.html file seems empty though:

https://output.circle-artifacts.com/output/job/b5e9f711-4506-4d76-a916-1a1bb2c13fe0/artifacts/0/doc/_changed.html

@ogrisel @thomasjpfan
Use the :term: directive.
530750a 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@alfaro96
Copy link
Member
alfaro96 commented Jun 4, 2022 •
edited
Loading

The _changed.html file seems empty though:

https://output.circle-artifacts.com/output/job/b5e9f711-4506-4d76-a916-1a1bb2c13fe0/artifacts/0/doc/_changed.html

I think that the problem is related with the following error: fatal: origin/main...530750af747f078608b7a4f240547101b986fc6b: no merge base [10736](https://github.com/scikit-learn/scikit-learn/runs/6739410120?check_suite_focus=true#step:4:10737). I am investigating this issue.

@alfaro96 alfaro96 mentioned this pull request Jun 4, 2022
Merged
thomasjpfan
thomasjpfan approved these changes Jun 5, 2022
View reviewed changes
build_tools/github/build_doc.sh
@@ -20,7 +20,8 @@ set -e
if [ -n "$GITHUB_ACTION" ]
then
# Map the variables for the new documentation builder to the old one
CIRCLE_SHA1=$(git log --no-merges -1 --pretty=format:%H)
CIRCLE_SHA1=$(git log -1 --pretty=format:%H)
Copy link
Member

Choose a reason for hiding this comment

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

Removing the --no-merges fixes the issue with finding the changes.

Note: I tried using GITHUB_SHA here: 8de2674 (#23534), but it didn't pick up the diff.

@ogrisel ogrisel merged commit 6848f41 into scikit-learn:main Jun 6, 2022
ogrisel added a commit to ogrisel/scikit-learn that referenced this pull request Jul 11, 2022
@lesteve @thomasjpfan @ogrisel
DOC fix glossary link (scikit-learn#23534)
7b7e501 

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Aug 4, 2022
@lesteve @thomasjpfan
@ogrisel @glemaitre
DOC fix glossary link (scikit-learn#23534)
e734134 

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
glemaitre pushed a commit that referenced this pull request Aug 5, 2022
@lesteve @thomasjpfan
@ogrisel @glemaitre
DOC fix glossary link (#23534)
4ab81d8 

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@lesteve lesteve deleted the fix-glossary-link branch March 31, 2023 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
Documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lesteve @alfaro96 @thomasjpfan @ogrisel
0