Add a filename-prefix option to the Sphinx plot directive #28187
Conversation
There was a problem hiding this comment.
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.
|
For anyone interested, the error you get if you try to use the plot directive with MyST directly is |
|
Ah, I didn't realize there actually already are unit tests for this. Let me update that. |
This allows specifying the output base name of the generated image files. The
name can include '{counter}', which is automatically string formatted to an
incrementing counter. The default if it is not specified is left intact as
the current behavior, which is to use the base name of the provided script or
the RST document.
This is required to use the plot directive with MyST, because the directive is
broken with MyST (an issue I don't want to fix), requiring the use of
eval-rst. But the way eval-rst works, the incrementing counter is not
maintained across different eval-rst directives, meaning if you try to include
multiple of them in the same document, the images will overwrite each other.
This allows you to manually work around this with something like
```{eval-rst}
.. plot::
:output-base-name: plot-1
...
```
```{eval-rst}
.. plot::
:output-base-name: plot-2
...
```
Aside from this, it's generally useful to be able to specify the image name
used for a plot, as a more informative name can be used rather than just
'<document-name>-1.png'.
|
For me, the Sphinx extension tests fail on main |
|
I added a test that will hopefully work. I'm having difficulty getting some of the tests to run successfully locally right now, but I believe the asserts I added work. If CI fails I might need some help figuring out why things aren't working on my computer. |
| else: | ||
| source_file_name = rst_file | ||
| code = textwrap.dedent("\n".join(map(str, content))) | ||
| counter = document.attributes.get('_plot_counter', 0) + 1 | ||
| document.attributes['_plot_counter'] = counter | ||
| base, ext = os.path.splitext(os.path.basename(source_file_name)) | ||
| output_base = '%s-%d.py' % (base, counter) | ||
| if options['output-base-name']: | ||
| output_base = options['output-base-name'].format(counter=counter) |
There was a problem hiding this comment.
could we prefix in the script/rst file name here as well? If you have duplicate output names in the same rst file that is a quick search to find and fix, but if you have the collision across multiple rst files it could be much more annoying to find where they are colliding.
There was a problem hiding this comment.
Well my thinking here is also that users should have full control over the filename that is produced. If we always inject something into the name, that won't be the case. For instance, if you right-click and save a plot, this will be the filename used for the image.
Really, we need to figure out how to error if the same name is used twice. My naive idea of checking if the file already exists doesn't work because it could just exist from a previous build. It's probably possible to keep a global list of already used filenames, but I'll have to figure out how to make that work with partial Sphinx rebuilds.
|
What version of pytest are you using? I could not reproduce this issue last night. |
|
I am 👍 on this idea, one small question about the naming (I see both sides of forcing an additional prefix or not). I re-milestoned this for 3.10 as it is a new feature which seems too big to put in via a micro release and it is too late to get this is under the wire for 3.9. |
It's possible my local build of matplotlib is broken somehow. pytest also does a really annoying thing when I run this test on my Mac where it switches to another space like it is opening a GUI window. I also had to run or the tests would just error completely with some strange error (which fortunately ChatGPT was able to figure out). I tried my best to follow your dev instructions, but some of the steps didn't work (e.g., the dev requirements.txt doesn't actually list any build requirements). |
|
The reason I suspect pytest is this path: What was the strange error? I'm really not sure why you need X11 stuff for the tests of the sphinx extension to run... We have an open PR to fix the dev install instructions. The conda environment file works, most of the regular devs who don't use conda know what to do, and we explicitly install them on CI so it fell through the cracks. |
|
Sorry that I wasn't clear. I reproduced the Sphinx issue outside of pytest, using The first output I showed is from pytest (which is running that command). The second is the log file from running the command directly, which shows the full traceback. |
|
Ah, so looking closer at https://matplotlib.org/devdocs/devel/development_setup.html#create-a-dedicated-environment, I see there is a secret "conda" button you have to click. I really don't like that Sphinx extension that hides content behind tabs... |
|
oh, I bet our tiny pages config needs the backend set to |
|
The extension supposedly does that But perhaps my private matplotlib config is messing with things. |
|
I have some work-in-progress code to do the duplicate filename checks. I'm trying to figure out why this matplotlib/lib/matplotlib/sphinxext/plot_directive.py Lines 484 to 490 in eb17273 |
|
By the way, I saw you approved this. If you want to merge this as-is, that's fine. I can add the duplicate filename check in a separate PR. I'm still not completely sure yet if I can actually get it working. |
|
So it occurs to me that this might actually do unexpected things if a global output_base_name is set when there are partial rebuilds. I think I might just remove the ability to have a global option, as well as the |
|
This is also going to do funny looking things if you mix plots with and without set base names as the counter is always increased so I think you will get names like [ One option would be to the Another option would be to instead of thinking of this as setting the base name, change the user facing knob to be a postfix so either we append a counter OR we stick on what ever the user gave us. I think that will fix the myst problem and prevents inter-document name collisions. |
Too enthusiastic, still some open questions.
|
My plan is to simplify this considerably:
|
If you're talking about |
|
Is this CI test failure related to my changes? |
|
No it's a flaky test. |
| def check_output_base_name(env, output_base): | ||
| docname = env.docname | ||
|
|
||
| if '.' in output_base or '/' in output_base: |
There was a problem hiding this comment.
Since this is passed through Path, etc., you should also check for \\.
|
I can't comment on the line directly since it's not part of the diff, but the |
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
|
I added a test for multiple figures. I can confirm that it does work by testing manually, but for some reason adding leads to a much earlier test failing. |
|
Good catch on the source link. It is working, but it looks like it isn't properly adding the |
|
Oh I think that was just because I was testing tinypages locally and it was messing with the tests. |
…docs in tinypages The tests now don't copy any of the build files, meaning the tests should pass even if tinypages has stale build state from a manual build.
|
OK, this is ready for review again. |
I don't care too much between We should consider whether basename is appropriate. While os.path.basename includes the extension, here it's not included. stem would be the technically correct term. However I feel Either way, I believe the logic is too complex to caputure everything in a single name. Until now, everything was magically determined behind the scenes (i.e. users should not have to bother with the exact name). If we now make the name configurable, we should explain what exactly this is in the associated docs (1) this must not have any path components |
|
My original version did have something more configurable (the user could specify the format string), but I decided that was way too complex for what was needed here. I think the main thing here is that the user has some way to make the name a little more human readable. If it also adds some numbering or file extensions to that it isn't a problem, especially given that no one else has even raised this as a problem before. And obviously for my original problem the name itself doesn't even matter; it just needs to be unique. |
|
Then possibly |
|
FWIW, none of the suggested names really make a ton of sense if you just see them in a |
|
|
|
OK, I've updated it to use |
This allows specifying the output base name of the generated image files.
This is required to use the plot directive with MyST, because the directive is broken with MyST (an issue I don't want to fix), requiring the use of eval-rst. But the way eval-rst works, the incrementing counter is not maintained across different eval-rst directives, meaning if you try to include multiple of them in the same document, the images will overwrite each other. This allows you to manually work around this with something like
Aside from this, it's generally useful to be able to specify the image name used for a plot, as a more informative name can be used rather than just
<document-name>-1.png.PR summary
PR checklist