Skip to content

MNT Do not update docs with deprecated decorator#24410

Merged
ogrisel merged 2 commits intoscikit-learn:mainfrom
thomasjpfan:deprecate_numpydocs
Oct 14, 2022
Merged

MNT Do not update docs with deprecated decorator#24410
ogrisel merged 2 commits intoscikit-learn:mainfrom
thomasjpfan:deprecate_numpydocs

Conversation

@thomasjpfan
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan commented Sep 9, 2022

Reference Issues/PRs

Fixes #24328

What does this implement/fix? Explain your changes.

This PR removes the _update_doc from the deprecated decorator. I did not update because they will be removed in 1.2:

  • All get_feature_names methods
  • load_boston
  • plot_confusion_matrix
  • plot_roc_curve
  • graph_shortest_path

@Kshitij68
Copy link
Copy Markdown
Contributor

Kshitij68 commented Sep 12, 2022

Going through the CI Logs for documentation build step, I see many warnings messages like

/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_cluster_comparison_001.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_kmeans_assumptions_001.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_kmeans_digits_001.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_mini_batch_kmeans_001.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_affinity_propagation_001.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_mean_shift_001.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_segmentation_toy_001.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_segmentation_toy_002.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_segmentation_toy_001.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_segmentation_toy_002.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_coin_segmentation_001.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_coin_segmentation_002.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_coin_segmentation_003.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_coin_segmentation_001.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_coin_segmentation_002.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_coin_segmentation_003.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images/sphx_glr_plot_linkage_comparison_001.png
/home/runner/work/scikit-learn/scikit-learn/doc/modules/clustering.rst:: WARNING: image file not readable: auto_examples/cluster/images

Are we sure that this is okay to be ignored.
My experience with sphinx has been that it silently drops stuff via warning and the only proper fix is to have a check in CI pipeline to ensure that warnings do not appear.

@thomasjpfan
Copy link
Copy Markdown
Member Author

thomasjpfan commented Sep 12, 2022 •
edited
Loading

Yup those are safe to ignore for pull requests, because we do not run the examples to generate the images by default. Running all the examples on every PR will greatly increase the build time especially if the PR does not touch any of the examples.

For PRs, we have code that catches warnings for files that were changed in the PR:

scikit-learn/build_tools/github/build_doc.sh

Line 230 in e04fb58

echo "The following documentation warnings may have been generated by PR #$CI_PULL_REQUEST:"

That file also has clever logic to make sure files that reference images in the examples will also run those examples.

jeremiedbb
jeremiedbb approved these changes Sep 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan thomasjpfan added the Quick Review For PRs that are quick to review label Sep 29, 2022
@jeremiedbb
Copy link
Copy Markdown
Member

jeremiedbb commented Oct 13, 2022

@thomasjpfan seems like you messed up you sync with main 😃

ogrisel
ogrisel approved these changes Oct 14, 2022
View reviewed changes
Copy link
Copy Markdown
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.

LGTM as well.

jjerphan
jjerphan approved these changes Oct 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you, @thomasjpfan.

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Oct 14, 2022

I confirm that all the remaining @deprecated annotations refer to attributes (properties) so there is no extra docstring to update.

ogrisel
ogrisel reviewed Oct 14, 2022
View reviewed changes
@ogrisel ogrisel merged commit eb5f340 into scikit-learn:main Oct 14, 2022
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 31, 2022
@thomasjpfan @ogrisel @glemaitre
MNT Do not update docs with deprecated decorator (scikit-learn#24410)
d0c3bf3 
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
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

@jjerphan jjerphan jjerphan approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees

No one assigned

Labels

module:linear_model module:metrics module:utils No Changelog Needed Quick Review For PRs that are quick to review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DOC deprecate is updating a docstring against numpydocs rules

5 participants

@thomasjpfan @Kshitij68 @jeremiedbb @ogrisel @jjerphan