DOC/BLD: plot directive srcset - #25515

So this builds, with a couple of weird errors that I'll track down.

https://output.circle-artifacts.com/output/job/7ea85200-9b06-4f15-b73b-bbfef3da4233/artifacts/0/doc/build/html/index.html

Discussion point:

The relatively large change this does is that image files had a funny naming convention using .. figure:: and I've made .. figure-mpl:: behave "differently".

Consider users/explain. We have subdirectories and they all have index.html in them. These index.html files currently all get called index-*.png with an increasing number:

users/explain/artists/index.rst:

https://matplotlib.org/devdocs/_images/index-1.png
https://matplotlib.org/devdocs/_images/index-2.png

users/explain/axes/index.rst:

https://matplotlib.org/devdocs/_images/index-11.png
https://matplotlib.org/devdocs/_images/index-21.png

users/explain/figures/index.rst:

https://matplotlib.org/devdocs/_images/index-12.png
https://matplotlib.org/devdocs/_images/index-22.png

Here, we had to implement the file copying ourselves, so I chose to do it:

users/explain/artists/index.rst:

https://matplotlib.org/devdocs/_images/users/explain/artists/index-1.png
https://matplotlib.org/devdocs/_images/users/explain/artists/index-2.png

users/explain/axes/index.rst:

https://matplotlib.org/devdocs/_images/users/explain/axes/index-1.png
https://matplotlib.org/devdocs/_images/users/explain/artists/index-2.png

users/explain/figures/index.rst:

https://matplotlib.org/devdocs/_images/users/explain/figures/index-12.png
https://matplotlib.org/devdocs/_images/users/explain/figures/index-22.png

So, this is a breaking change. OTOH I think this is a lot better than putting 1000s of images in a flat directory, all with almost the same names. Not sure what our policy is for breaking changes on the plot directive...

To be clear nothing is changing on the user input side (unless they want to use the new feature) but what is changing in the location of the output files. If the user is letting us / sphinx do everything then they will not notice.

If they have hard-coded direct urls to these output files then things might break. However, if I am understanding the previous behavior correctly the name depended on the order in which the files were processed and the number of .. plot directives in any earlier files. If that is the case, then the filenames were wildly unstable anyway and this change will make them more stable going forward.

I am 👍 on making this change (with a note). I am however a bit confused why we need a new .. figmpl rather than updating .. figure? I would be happy with "because it is way easier", just checking if there is some other reason.

I am however a bit confused why we need a new .. figmpl rather than updating .. figure?

We don't provide .. figure::. It is supplied by docutils, which adds an image node that gets handled by docutils+sphinx. They just copy everything into imagedir (build/html/_images/ for us). We need to take over the file copying because we need to copy more files.

One concern I haven't been brave enough to test yet is that nesting the image files won't work for singlehtml. I don't know who uses that, or why it exists. Another naming convention could be sub/dir/index.rst:first-figure -> sub_dir_index-1.png. The default sphinx/docutils way of handling this is really pretty ugly and, as you say, unstable.

I just realized that I think we can keep the old behaviour with minimal complexity by just using a different TEMPLATE if the downstream user isn't using eg plot_srcset='2x'. So users who want the srcset functionality will get the new non-standard image organization, but users who don't set this will get exactly the old behaviour. I'll implement that the next few days.

The question remains if it should be _images/sub/dir/index-1.png or _images/sub-dir-index-1.png. I guess I'm starting to lean towards the latter..

https://output.circle-artifacts.com/output/job/5e6a201b-625c-43f6-b99a-be0272981d5a/artifacts/0/doc/build/html/users/explain/axes/index.html the image links to

build/html/_images/users-explain-axes-index-1.2x.png now....

I'll agitate for this a bit - it has no downstream user-facing changes unless the user decides to use the srcset option; it also has no internal changes unless that option is specified. So I think this is pretty low risk to put in, for a pretty helpful benefit to making the docs look a lot better.

Thanks for your help @QuLogic !

Appveyor test failure does appear to be the newly added test.

I'm mildly suspicious of the PurePosixPath usage, when interacting with a real file system, and how that might work on windows...

It appears to work on the Azure windows builds (unless it is skipped somehow, but I didn't see it in the list of skipped tests), so not really sure

FAILED lib/matplotlib/tests/test_sphinxext.py::test_srcset_version - AssertionError: sphinx build failed with stdout:
  Running Sphinx v5.3.0
  making output directory... done
  building [mo]: targets for 0 po files that are out of date
  building [html]: targets for 5 source files that are out of date
  updating environment: [new config] 5 added, 0 changed, 0 removed
  reading sources... [ 20%] included_plot_21                                     
  reading sources... [ 40%] index                                                
  reading sources... [ 60%] nestedpage/index                                     
  reading sources... [ 80%] nestedpage2/index                                    
  reading sources... [100%] some_plots                                           
  
  looking for now-outdated files... none found
  pickling environment... done
  checking consistency... done
  preparing documents... done
  writing output... [ 20%] included_plot_21                                      
  
  stderr:
  
  Exception occurred:
    File "C:\Miniconda3-x64\envs\mpl-dev\lib\shutil.py", line 264, in copyfile
      with open(src, 'rb') as fsrc:
  FileNotFoundError: [Errno 2] No such file or directory: 'plot_directive/included_plot_21-1.png'
  The full traceback has been saved in C:\Users\appveyor\AppData\Local\Temp\1\sphinx-err-3x441lvn.log, if you want to report the issue to the developers.
  Please also report this if it was a user error, so that a better error message can be provided next time.
  A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
  
  
assert 2 == 0
 +  where 2 = CompletedProcess(args=['C:\\Miniconda3-x64\\envs\\mpl-dev\\python.exe', '-msphinx', '-W', '-b', 'html', '-d', 'C:\\Use...ided next time.\nA bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!\n').returncode

Windows accepts either path separator; this failure indicates either the image isn't created (yet?), is named differently, or the working directory (since this is a relative path) is not the expected one.

That being said, we probably don't need to use PurePosixPath as input to copyfile; it's only really important for URLs since those don't accept \.

Huh, I've never ever looked at appveyor - sorry I assumed the error was spurious.

Sadly I don't have a windows machine to test on. Happy to make any changes suggested by others.

The failure on appveyor is real, but I don't have any rational way to debug it :-(

Some interactive backend tests are failing, which I don't think have to do with this PR...

I think #25988 is tracking those test failures.