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.

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

sphinx: enable nit-picky mode #570

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

sphinx: enable nit-picky mode #570

wants to merge 1 commit into from

Conversation

romintomasetti
Copy link
Contributor

@romintomasetti romintomasetti commented Aug 27, 2024 •
edited
Loading

Nit-picky mode will shout at any missing reference.

@romintomasetti romintomasetti self-assigned this Aug 27, 2024
@romintomasetti romintomasetti mentioned this pull request Aug 27, 2024
Merged
masterleinad
masterleinad requested changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

We already have -W in that line. Does it need to be in a different position?

@romintomasetti
Copy link
Contributor Author

Oh... Then you probably just miss -n 😄

@romintomasetti romintomasetti changed the title sphinx: turn warnings into errors sphinx: enable nit-picky mode Aug 27, 2024
masterleinad
masterleinad requested changes Aug 28, 2024
View reviewed changes
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

We'll have to fix all the new warnings here.

@romintomasetti
Copy link
Contributor Author

@masterleinad I think most of the references are broken when it comes to the C++ domain stuff.

Here is what I get e.g. in docs/source/API/core/view/view.rst:

writing output... [100%] index                                                                                                                                                                                                                                      
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:243: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:269: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:296: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:320: WARNING: cppkokkos:identifier reference target not found: ScratchSpace
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:320: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:333: WARNING: cppkokkos:identifier reference target not found: ScratchSpace
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:343: WARNING: cppkokkos:identifier reference target not found: DT
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:343: WARNING: cppkokkos:identifier reference target not found: Prop
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:343: WARNING: cppkokkos:identifier reference target not found: Args
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:347: WARNING: cppkokkos:identifier reference target not found: traits::is_managed
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:347: WARNING: cppkokkos:identifier reference target not found: NATURAL_MDSPAN_TYPE
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:359: WARNING: cppkokkos:class reference target not found: View
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:359: WARNING: cpp:any reference target not found: mds
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:359: WARNING: cppkokkos:class reference target not found: View
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:365: WARNING: cppkokkos:identifier reference target not found: SEE_BELOW
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:365: WARNING: cppkokkos:identifier reference target not found: mdspan<ElementType, ExtentsType, LayoutType, AccessorType>
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:379: WARNING: cppkokkos:class reference target not found: View
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:379: WARNING: cpp:any reference target not found: mds
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:379: WARNING: cppkokkos:class reference target not found: View
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:379: WARNING: cpp:any reference target not found: mds
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:379: WARNING: cppkokkos:class reference target not found: View
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:383: WARNING: cpp:any reference target not found: mds
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:383: WARNING: cppkokkos:class reference target not found: View
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:391: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:398: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:398: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:398: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:398: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:398: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:398: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:398: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:398: WARNING: cppkokkos:identifier reference target not found: IntType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:410: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:414: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:426: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:438: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:443: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:447: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:451: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:455: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:459: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:463: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:467: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:471: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:480: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:486: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:490: WARNING: cppkokkos:identifier reference target not found: View
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:501: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:501: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:501: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:501: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:501: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:501: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:501: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:501: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:501: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:501: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:508: WARNING: cppkokkos:identifier reference target not found: size_t
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:523: WARNING: cppkokkos:identifier reference target not found: View<DataType, Properties...>
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:523: WARNING: cppkokkos:identifier reference target not found: DataType
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:523: WARNING: cppkokkos:identifier reference target not found: Properties
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:544: WARNING: cppkokkos:identifier reference target not found: mdspan<OtherElementType, OtherExtents, OtherLayoutPolicy, OtherAccessor>
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:551: WARNING: cppkokkos:class reference target not found: View
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:553: WARNING: cppkokkos:class reference target not found: View
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:555: WARNING: cppkokkos:identifier reference target not found: Kokkos
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:555: WARNING: cppkokkos:identifier reference target not found: Kokkos::default_accessor<typename traits::value_type>
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:555: WARNING: cppkokkos:identifier reference target not found: traits::value_type
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:561: WARNING: cppkokkos:class reference target not found: View
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:561: WARNING: cpp:any reference target not found: other_accessor
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:638: WARNING: cppkokkos:class reference target not found: View
/workspaces/kokkos-core-wiki/docs/source/API/core/view/view.rst:638: WARNING: cppkokkos:class reference target not found: View

Some make sense (missing template <...> things to make it happy), others don't (like cppkokkos:class reference target not found: View).

I tried updating some of the requirements to see if it helped, but not much apparently.

Any idea how to proceed with this ?

@masterleinad
Copy link
Contributor

It seems you easily get these kinds of spurious warnings, see, e.g., https://stackoverflow.com/questions/76742702/sphinx-warning-reference-target-not-found-for-standard-c-types, breathe-doc/breathe#696, or https://stackoverflow.com/questions/11417221/sphinx-autodoc-gives-warning-pyclass-reference-target-not-found-type-warning.
The only workaround is to add all types to nitpick_ignore.

What do we really gain from enabling nit-picky mode. Is it worth it to keep a long list for nitpick_ignore to enable it?

@romintomasetti
Copy link
Contributor Author

I think most of links like

:cppkokkos:class:`...`

that are broken are still rendering good, but are not clickable.

I guess that nit-picky mode is mostly that: ensuring clickable stuff is clickable.

ajpowelsnl
ajpowelsnl reviewed Aug 30, 2024
View reviewed changes
Copy link
Contributor

@ajpowelsnl ajpowelsnl left a comment

Choose a reason for hiding this comment

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

My comments are general at this point:

  • Did you find a surfeit of broken links warranting "nit-picky" as the default? Either way, we might want a build option for "nit-picky" vs. not "nit-picky," if we were to move towards adoption
  • Please make sure the PR does one and only one thing, i.e., proposing "nit-picky" as a code change (vs. adopting "nit-picky" + all the fixes "nit-picky" might require)

build_requirements.txt
furo==2023.09.10
myst-parser==3.0.0
sphinx-copybutton==0.5.2
sphinx-design==0.6.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Were these particular versions selected for a reason, apart from their (possibly) being the most recent in a package manager? If there's no real reason for these particular versions, would it make sense to take your package manager's (conda, pip) default? Taking the defaults would spare us maintaining hard-coded versions.

docs/Makefile
@@ -1,5 +1,5 @@
html:
sphinx-build -b html ./source/ ./generated_docs/ -W --keep-going
sphinx-build -b html ./source/ ./generated_docs/ -W -n --keep-going
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to write a little bloc saying that you are building in "nit-picky" mode? Would we want "nit-picky" to be our default condition? If yes, then we're accepting the possibility of the mass of broken links outweighing the occurrence of spurious / unhelpful warnings / errors. Do we have any broken links : unhelpful warnings data?

docs/source/API/algorithms/std-algorithms/Iterators.rst

Returns a Kokkos **random access** iterator to the beginning of ``view``

.. cppkokkos:kokkosinlinefunction:: template <class DataType, class... Properties> auto cbegin(const Kokkos::View<DataType, Properties...>& view);
.. cppkokkos:kokkosinlinefunction:: template <class DataType, class... Properties> auto cbegin(const View<DataType, Properties...>& view);

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes in lines 13 and 17 "nit-picky" related, or are you just correcting errors you happened upon?

docs/source/API/algorithms/std-algorithms/Iterators.rst

Returns a Kokkos **random access** iterator to the element past the end of ``view``

.. cppkokkos:kokkosinlinefunction:: template <class DataType, class... Properties> auto cend(const Kokkos::View<DataType, Properties...>& view);
.. cppkokkos:kokkosinlinefunction:: template <class DataType, class... Properties> auto cend(const View<DataType, Properties...>& view);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes in lines 13, 17, 21 and 25 "nit-picky" related, or are you just correcting errors you happened upon?

docs/source/API/core/view/view.rst
@@ -12,10 +12,9 @@ Its semantics are similar to that of ``std::shared_ptr``.
Interface
---------

.. code-block:: cpp
.. cppkokkos:class:: template <typename DataType> View
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

docs/source/API/core/view/view.rst

template <class DataType [, class LayoutType] [, class MemorySpace] [, class MemoryTraits]>
class View;
We should still describe DataType LayoutType MemorySpace MemoryTraits....
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean for line 17 to be part of the documentation, or is it intended as a "TODO"?

docs/source/API/core/view/view.rst
@@ -115,6 +114,10 @@ Typedefs

.. rubric:: Data Types

.. cppkokkos:type:: traits

To do.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intent to provide a brief description of traits? If so, were you planning do it in this PR : )

docs/source/API/core/view/view.rst
@@ -263,7 +266,7 @@ Constructors
either match the dynamic rank or the total rank. In the latter case, the extents
corresponding to compile-time dimensions must match the View type's compile-time extents.

.. cppkokkos:function:: View( const ALLOC_PROP &prop, const IntType& ... indices)
.. cppkokkos:function:: View::View( const ALLOC_PROP &prop, const IntType& ... indices)
Copy link
Contributor

Choose a reason for hiding this comment

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

ALLOC_PROP: perhaps an opportunity to link to the forthcoming Glossary of terms championed by @nmm0 !

docs/source/API/core/view/view.rst

Subview constructor. See ``subview`` function for arguments.

.. cppkokkos:function:: explicit(traits::is_managed) View( const NATURAL_MDSPAN_TYPE& mds )
.. cppkokkos:function:: explicit(traits::is_managed) View::View( const NATURAL_MDSPAN_TYPE& mds )
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a definition somewhere for NATURAL_MDSPAN_TYPE?

@JBludau
Copy link
Contributor

JBludau commented Aug 31, 2024

It seems you easily get these kinds of spurious warnings, see, e.g., https://stackoverflow.com/questions/76742702/sphinx-warning-reference-target-not-found-for-standard-c-types, breathe-doc/breathe#696, or https://stackoverflow.com/questions/11417221/sphinx-autodoc-gives-warning-pyclass-reference-target-not-found-type-warning. The only workaround is to add all types to nitpick_ignore.

What do we really gain from enabling nit-picky mode. Is it worth it to keep a long list for nitpick_ignore to enable it?

hmm, I think I would not bother about getting it to work with nit-picky mode. I don't see a big use in clickable return types for example but it would add to the pile of the developers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajpowelsnl ajpowelsnl ajpowelsnl left review comments

@masterleinad masterleinad masterleinad requested changes

@dalg24 dalg24 Awaiting requested review from dalg24

Requested changes must be addressed to merge this pull request.

Assignees

@romintomasetti romintomasetti

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@romintomasetti @masterleinad @JBludau @ajpowelsnl