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

Address Sphinx warnings #2758

Merged
merged 26 commits into from Dec 27, 2022
Merged

Address Sphinx warnings #2758

merged 26 commits into from Dec 27, 2022

Conversation

binste
Copy link
Contributor

@binste binste commented Dec 17, 2022

I get quite a few warnings from Sphinx when building the docs. I'll try to address them in this PR. It's not ready yet to be merged but wanted to provide visibility that I'm working on this so we're not duplicating efforts

  • recommonmark produces many UserWarnings which do not seem to be helpful. Package is deprecated, see their archived repo and it's recommended to switch to myst.
  • "Raw content disabled." -> Emitted by myst parse. myst sets raw_enabled to False per default in Parser.setup_parse. this worked with previous parser. set it explicitly else raw html content won't show up which it should. E.g. it would show up as image although that span should be blue: image
  • warnings that gallery examples are not included in any toctree -> mark as "orphans" https://stackoverflow.com/a/13109215
  • AltairDeprecationWarning: Use 'value' instead of 'init'.
  • AltairDeprecationWarning: The value of 'empty' should be True or False.
  • AltairDeprecationWarning: 'add_selection' is deprecated. Use 'add_params' instead.
  • AltairDeprecationWarning: The types 'single' and 'multi' are now combined and should be specified using "type='point'".
  • /workspaces/altair/doc/releases/changes.rst:114: WARNING: undefined label: 'user-guide-image-mark'
  • autosummary fails for some classes. See comment below.
  • Many warnings on formatting of restructuredtext. Come from a list entry starting with - instead of *
/workspaces/altair/altair/vegalite/v5/schema/channels.py:docstring of altair.vegalite.v5.schema.channels.Radius:136: WARNING: Bullet list ends without a blank line; unexpected unindent.
/workspaces/altair/altair/vegalite/v5/schema/channels.py:docstring of altair.vegalite.v5.schema.channels.Radius:136: WARNING: Inline interpreted text or phrase reference start-string without end-string.
/workspaces/altair/altair/vegalite/v5/schema/channels.py:docstring of altair.vegalite.v5.schema.channels.Radius:137: WARNING: Bullet list ends without a blank line; unexpected unindent.
/workspaces/altair/altair/vegalite/v5/schema/channels.py:docstring of altair.vegalite.v5.schema.channels.Radius:145: ERROR: Unexpected indentation.
...
  • Duplicate definitions
/workspaces/altair/altair/vegalite/v5/schema/channels.py:docstring of altair.vegalite.v5.schema.channels.Color:1: WARNING: duplicate object description of altair.Color, other instance in user_guide/generated/channels/altair.Color, use :noindex: for one of them
/workspaces/altair/altair/vegalite/v5/schema/channels.py:docstring of altair.vegalite.v5.schema.channels.Color:1: WARNING: duplicate object description of altair.vegalite.v5.schema.channels.Color, other instance in user_guide/generated/channels/altair.Color, use :noindex: for one of them
/workspaces/altair/altair/vegalite/v5/schema/channels.py:docstring of altair.vegalite.v5.schema.channels.Color.__init__:1: WARNING: duplicate object description of altair.Color.__init__, other instance in user_guide/generated/channels/altair.Color, use :noindex: for one of them
/workspaces/altair/altair/vegalite/v5/schema/channels.py:docstring of altair.vegalite.v5.schema.channels.Text:1: WARNING: duplicate object description of altair.Text, other instance in user_guide/generated/channels/altair.Text, use :noindex: for one of them
/workspaces/altair/altair/vegalite/v5/schema/channels.py:docstring of altair.vegalite.v5.schema.channels.Text:1: WARNING: duplicate object description of altair.vegalite.v5.schema.channels.Text, other instance in user_guide/generated/channels/altair.Text, use :noindex: for one of them
/workspaces/altair/altair/vegalite/v5/schema/channels.py:docstring of altair.vegalite.v5.schema.channels.Text.__init__:1: WARNING: duplicate object description of altair.Text.__init__, other instance in user_guide/generated/channels/altair.Text, use :noindex: for one of them
  • /workspaces/altair/doc/user_guide/generated/toplevel/altair.Chart.rst:: ERROR: Anonymous hyperlink mismatch: 2 references but 0 targets. See "backrefs" attribute for IDs.
  • schema_description:9: ERROR: Unknown target name: "types#datetime".

@binste binste mentioned this pull request Dec 17, 2022
@binste
Copy link
Contributor Author

binste commented Dec 19, 2022

I'll use this comment to explain the reasoning behind pinning setuptools to < 64 and will add a link to it in the pyproject.toml file so it's easier to revisit this decision in the future.

The PR pypa/setuptools#3265 (released in version 64) changed how Python packages are installed by setuptools in editable mode. This change, presumably unintentionally, has the side effect that importlib.import_module seems to now look for a module case insensitive, e.g. importlib.import_module("altair.Datasets") imports the module altair/datasets.py. Due to this, the sphinx autosummary extension can no longer import methods of classes for which a module exists with the same name as the class but lowercase (sphinx uses importlib in the function sphinx.ext.autosummary.import import_by_name. Currently, this affects altair.Datasets and altair.Expr.

When using setuptools >= 64 you'll see the following errors in the console when building the docs:

/workspaces/altair/doc/user_guide/generated/core/altair.Datasets.rst:14: WARNING: autosummary: failed to import Datasets.copy.
Possible hints:
* ModuleNotFoundError: No module named 'altair.Datasets.copy'; 'altair.Datasets' is not a package
* ModuleNotFoundError: No module named 'Datasets'
* AttributeError: module 'altair.Datasets' has no attribute 'Datasets'
* AttributeError: module 'altair.Datasets' has no attribute 'copy'
* KeyError: 'Datasets'
* ImportError: 
* ModuleNotFoundError: No module named 'altair.Datasets.Datasets'; 'altair.Datasets' is not a package

And the resulting page will look like this:
image

After pinning setuptools to < 64 it works again:

image

This decision should be revisited in the future. Somewhat related discussion: scikit-build/scikit-build#740

@binste
Copy link
Contributor Author

binste commented Dec 20, 2022

Commit 7ef558d fixes docstrings where attributes had multiple types which were wrapped onto a new line.

condition : anyOf(:class:`ConditionalValueDefnumberExprRef`,
List(:class:`ConditionalValueDefnumberExprRef`))
    One or more value definition(s) with `a selection or a test predicate

Sphinx renders this as if List(:class... is a new attribute:
image
(FillOpacityDatum)

After the fix, these lines are no longer wrapped in the code:

condition : anyOf(:class:`ConditionalValueDefnumberExprRef`, List(:class:`ConditionalValueDefnumberExprRef`))
    One or more value definition(s) with `a selection or a test predicate

and therefore render ok in the docs (the line break below is due to the width of the page but you can see that the formatting changed):
image

@binste binste marked this pull request as ready for review December 21, 2022 07:17
@binste
Copy link
Contributor Author

binste commented Dec 21, 2022

I fixed various formatting issues in the docs as well as upstream in the docstring generation. For me the docs now build with only 1 warning left (see below) which is very satisfying! :) PR is ready for review and can also be merged without addressing the comment below.

@mattijn The remaining warning is <string>:3: UserWarning: Geometry is in a geographic CRS. Results from 'centroid' are likely incorrect. Use 'GeoSeries.to_crs()' to re-project geometries to a projected CRS before this operation. I assume it comes from the line geometry=gdf_sel.geometry.centroid in the geoshape mark page but I'm not familiar enough with it to understand what's happening. Could you take a quick look? Found this answer on stackexchange which might help.

binste and others added 3 commits December 21, 2022 07:44
This PR suppress the following warning to use a projected CRS for geometric computations.
```
<string>:3: UserWarning: Geometry is in a geographic CRS. Results from 'centroid' are likely incorrect. Use 'GeoSeries.to_crs()' to re-project geometries to a projected CRS before this operation.
```
For the given example we only need a _visual_ centroid and not an _exact_ centroid so this merely promotes good practice than being a necessity.
@mattijn
Copy link
Contributor

mattijn commented Dec 21, 2022

Looks great @binste!
One comment, the changes you applied to vega/v5/schema/core.py will be overwritten the next time the altair code is generated. You'll have to apply the changes in tools/generate_schema_wrapper.py as is mentioned at the top of the file:

# The contents of this file are automatically written by
# tools/generate_schema_wrapper.py. Do not modify directly.

@binste
Copy link
Contributor Author

binste commented Dec 21, 2022

Yep that's already the case. I made all those changes by modifying and then running tools/generate_schema_wrapper.py :) Same for the vegalite schema files. The respective changes to the generator script and the resulting changes to the schema files are bundled in the same commits so it's hopefully easier to review.

@mattijn
Copy link
Contributor

mattijn commented Dec 21, 2022

Ah, I was expecting to see changes in tools/generate_schema_wrapper.py but it seems you only had to apply changes to tools/schemapi/utils.py in this commit 7ef558d#diff-ed138919703443179382e2761f4e08ae579ea9f51d88f29ee59745de1e7a96be.

@binste
Copy link
Contributor Author

binste commented Dec 22, 2022

Indeed, sorry for the confusion!

@mattijn mattijn self-requested a review December 24, 2022 14:56
@mattijn mattijn merged commit 301c0af into vega:master Dec 27, 2022
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.

None yet

2 participants