Skip to content

feat(plot_directive): add RST directory option #26099

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

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

Conversation

jeertmans
Copy link

@jeertmans jeertmans commented Jun 9, 2023

Hello!

PR summary

After trying to set up my own documentation, I had trouble making the plot_directive work with the autodoc2 extension.

autodoc2 is unlike sphinx.ext.autodoc because it automatically generates all RST files, at a place that is not the same where we would usually call autodoc's directives.

To cope with this problem, I had to implement my own Sphinx directive, based on yours.

This may not be the perfect solution, but it works fine.

Feel free to suggest any change :-)

If you want to see a live example of this, here is the repo I developed the solution for: https://github.com/jeertmans/DiffeRT2d.

This example plot is available here: http://eertmans.be/DiffeRT2d/apidocs/differt2d/differt2d.scene.html#differt2d.scene.Scene.basic_scene.

EDIT: documentation examples are no longer online, but you may see the repository at the specific version the error was found.

PR checklist

jeertmans added 2 commits June 9, 2023 16:41
Hello!

After trying to set up my own documentation, I had trouble making the `plot_directive` work with the `autodoc2` extension.

`autodoc2` is unlike `sphinx.ext.autodoc` because it automatically generates all RST files, at a place that is not the same where we would usually call `autodoc`'s directives.

To cope with this problem, I had to implement my own Sphinx directive, based on yours.

This may not be the perfect solution, but it works fine.

Feel free to suggest any change :-)

If you want to see a live example of this, here is the repo I developed the solution for:  https://github.com/jeertmans/DiffeRT2d
Copy link

@github-actions github-actions bot 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 opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@story645 story645 added the Documentation: build building the docs label Jun 9, 2023
@tacaswell
Copy link
Member

Trying to understand this, it looks like the local variable rst_dir is used generate the path that is eventually passed to

result = jinja2.Template(config.plot_template or template).render(
default_fmt=default_fmt,
build_dir=build_dir_link,
src_name=src_name,
multi_image=len(images) > 1,
options=opts,
srcset=srcset,
images=images,
source_code=source_code,
html_show_formats=config.plot_html_show_formats and len(images),
caption=caption)
as build_dir=build_dir_link. Given that, I think that if we do take this the name should be something like plot_build_directory to be clearer about what it is actually affecting (rst_dir makes sense in the code as it is extracted from the path to the rst file).

I am also a little confused by why this is needed. Is autodoc2 generating rst in a non-final location, running a pass of processing on them, and then moving the output someplace else?

We are trying to infer the build directory, is there a better way to get that directly?

jeertmans and others added 2 commits June 9, 2023 21:31
Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
@jeertmans
Copy link
Author

Trying to understand this, it looks like the local variable rst_dir is used generate the path that is eventually passed to

result = jinja2.Template(config.plot_template or template).render(
default_fmt=default_fmt,
build_dir=build_dir_link,
src_name=src_name,
multi_image=len(images) > 1,
options=opts,
srcset=srcset,
images=images,
source_code=source_code,
html_show_formats=config.plot_html_show_formats and len(images),
caption=caption)

as build_dir=build_dir_link. Given that, I think that if we do take this the name should be something like plot_build_directory to be clearer about what it is actually affecting (rst_dir makes sense in the code as it is extracted from the path to the rst file).

Well this is not the build dir neither. Because the build directory is another place, but that also depends on the filename… For example, the build dir’s default location with my setup is put inside the docs/source dir, not the docs/build dir, which seems weird to me too.

I am also a little confused by why this is needed. Is autodoc2 generating rst in a non-final location, running a pass of processing on them, and then moving the output someplace else?

Autodoc2 generates source files, but is hardly configurable. So a solution could be to modify their extension by adding more flexibility, or adding some kind of post processor.

To me, the easiest solution was just to add a new configuration variable to the matplotlib extension. In my opinion, but the matplotlib extension and the autodoc2 extension could be a bit more configurable :)

We are trying to infer the build directory, is there a better way to get that directly?

Maybe, I tried to read a bit the code but it is quite hard to determine the implication of each variable :’-)


.. code:: python

plot_rst_directory = "source/apidocs/<your_package>/"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we can explain the folder situation here? Is this guaranteed to be always under the API docs source dir? I'm thinking of a potential mix between api and narrative docs and where the plots would live for this intermediate state.

@tacaswell
Copy link
Member

I have a memory of leaving another comment here but apparently not 😞


How does this interact with #24205 and is this related to the issue discussed in
#24005

I am still very skeptical of this as-is.

I think the analysis we need is a flow diagram of how the whole sphinx build process works here (I think we generate rst + images that get put in the source in an early step and then a late step goes discovers them and then generates the html which is why the "builddir" of this step is the "sourcedir" of the next step). There is clearly something wrong with the logic to generate the various paths (and the internal variable names could be improved), but adding a global setting to reach in and override a particular bit of the logic is unlikely to be the best solution. Even if we do have to do something like that, I remain skeptical that this is is the right place to put the knob.

@jklymak
Copy link
Member

jklymak commented Jun 15, 2023

I was just working on the directive so I will take a look in the next day or so.

@jklymak jklymak added the status: needs clarification Issues that need more information to resolve. label Jun 15, 2023
@jklymak
Copy link
Member

jklymak commented Jun 15, 2023

What happens is explained at https://github.com/matplotlib/matplotlib/blame/67c4bb3986e3bccfcebc69d1177007d4aa4483d2/lib/matplotlib/sphinxext/plot_directive.py#L154. I was going to say it is well-explained, but I wrote it, so I guess I think just like the author.

If we have an rst file in doc/my_info/boo.rst then the directive makes a plot in build/plot_directive/my_info/boo-1.png, and while it makes the html for build/my_info/boo.html it turns the boo.rst plot into a .. figure: directive that then puts the png into the webpage. This is done at

build_dir = os.path.join(os.path.dirname(setup.app.doctreedir),
                             'plot_directive',
                             source_rel_dir)

For our API data, it generates rst files under doc/api and any plot directives follow the same procedure as above.

I don't understand why the current PR wants to change rst_dir. That is only used for linking. I'd have to better understand where autodoc2 is trying to put things to understand what the issue is. But I'd have thought it would build a subdirectory of rst files under your doc directory, and then things should just work. If thats not the case, maybe @jeertmans can explain further.

Co-authored-by: Melissa Weber Mendonça <melissawm@gmail.com>
@jeertmans
Copy link
Author

If think the issue here is that autodoc2 actually generates the .rst files at build time, unlike autodoc that is just a Sphinx directive. When I have time, I'd like to work on a MWE, so it's easier to understand when it fails to work as expected.

@jklymak jklymak marked this pull request as draft January 2, 2025 19:14
@jklymak
Copy link
Member

jklymak commented Jan 2, 2025

I've moved to draft, but feel free to put back in the queue if you think this is ready,.

@jeertmans jeertmans closed this Jan 28, 2025
@jeertmans jeertmans deleted the patch-1 branch January 28, 2025 11:35
@jeertmans jeertmans restored the patch-1 branch January 28, 2025 11:36
@jeertmans jeertmans reopened this Jan 28, 2025
jeertmans added a commit to jeertmans/matplotlib-mwe-26099 that referenced this pull request Jan 28, 2025
@jeertmans
Copy link
Author

Hi @jklymak, I am marking as ready as I have finally found some time to create a MWE: https://github.com/jeertmans/matplotlib-mwe-26099.

GitHub actions are automatically run, and the latest ones show that apply the fix is necessary to build the documentation correctly.

See build logs when the fix is not applied:

/home/runner/work/matplotlib-mwe-26099/matplotlib-mwe-26099/src/matplotlib_mwe_26099/__init__.py:12: WARNING: image file not readable: docs/src/matplotlib_mwe_26099/__init__-2.png [image.not_readable]
/home/runner/work/matplotlib-mwe-26099/matplotlib-mwe-26099/src/matplotlib_mwe_26099/__init__.py:-10: WARNING: download file not readable: /home/runner/work/matplotlib-mwe-26099/matplotlib-mwe-26099/docs/source/docs/src/matplotlib_mwe_26099/__init__-2.py [download.not_readable]
/home/runner/work/matplotlib-mwe-26099/matplotlib-mwe-26099/src/matplotlib_mwe_26099/__init__.py:-10: WARNING: download file not readable: /home/runner/work/matplotlib-mwe-26099/matplotlib-mwe-26099/docs/source/docs/src/matplotlib_mwe_26099/__init__-2.png [download.not_readable]
/home/runner/work/matplotlib-mwe-26099/matplotlib-mwe-26099/src/matplotlib_mwe_26099/__init__.py:-10: WARNING: download file not readable: /home/runner/work/matplotlib-mwe-26099/matplotlib-mwe-26099/docs/source/docs/src/matplotlib_mwe_26099/__init__-2.hires.png [download.not_readable]
/home/runner/work/matplotlib-mwe-26099/matplotlib-mwe-26099/src/matplotlib_mwe_26099/__init__.py:-10: WARNING: download file not readable: /home/runner/work/matplotlib-mwe-26099/matplotlib-mwe-26099/docs/source/docs/src/matplotlib_mwe_26099/__init__-2.pdf [download.not_readable]
looking for now-outdated files... none found

@jeertmans jeertmans marked this pull request as ready for review January 28, 2025 12:04
@jeertmans jeertmans requested a review from tacaswell February 24, 2025 10:48
@jeertmans jeertmans requested a review from melissawm February 24, 2025 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs clarification Issues that need more information to resolve. topic: sphinx extension
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

5 participants