Fix image handling for RTD builds #260

pritkc · 2025-12-17T09:40:44Z

What type of PR is this? (check all applicable)

Description

This PR fixes an issue where images worked correctly on branch RTD builds but failed when merged to main. The solution implements automatic image handling that works consistently across all build environments (local, branch RTD, and main RTD).

Key Changes:

Implemented automatic image collection from multiple source locations (doc/assets/img/, figures/ directories, etc.)
Added path fixing for included markdown files at both source and doctree levels
Changed from config-inited to builder-inited event for more reliable timing
Added build output copying to ensure images are available in final HTML
Fixed HTML paths to use relative paths for local file viewing
Updated README with image handling instructions for contributors

Technical Details:
The fix uses Sphinx event hooks to:

Collect images from various locations before build starts (builder-inited event)
Fix image paths in source markdown before MyST processes includes (source-read event)
Fix broken paths in doctree after MyST processing (doctree-read event)
Copy images to build output directory (build-finished event)
Convert absolute paths to relative paths in HTML (build-finished event)

The solution is generic and works for any contributor adding images anywhere in the documentation, without requiring special configuration or knowledge of Sphinx internals.

Related Issues & Documents

Closes OSE Organization figures missing from HTML document #222

QA Instructions, Screenshots, Recordings

RTD Hosted link -- https://mole-form.readthedocs.io/en/main/

Testing Performed:

Local Build Testing: Verified images work correctly with make doc-html
RTD Branch Build: Tested on personal fork branch - images display correctly
RTD Main Build: Merged to fork's main branch - images work correctly
Multiple Image Locations: Tested images from:
- doc/assets/img/ (OSE_ORGANIZATION.md images)
- source/math_functions/figures/ (StaggeredGrids.md, CSRCReportOnMOLE.md)
- source/api/examples/md/figures/ (example figures)
- Section-specific figures/ directories

How to Test:

Build documentation locally:

cd doc/sphinx
make doc-clean
make doc-html

Verify images appear in build/html/ output
Check RTD builds (branch and main) to ensure images display correctly
Test adding a new image:
- Place image in doc/assets/img/ or a figures/ directory
- Reference it in markdown: ![Alt text](doc/assets/img/image.png)
- Build and verify it appears correctly

Screenshots:

Images now display correctly on RTD main builds (previously broken)

- No effect to latexpdf

Added/updated tests?

Yes
No, and this is why: please replace this line with details on why tests
have not been included
I need help with writing tests

Read Contributing Guide and Code of Conduct

I have read and followed the Contributing Guide and the Code of Conduct

[optional] Are there any post deployment tasks we need to perform?

No post-deployment tasks required. The fix is automatic and works for all existing and future images.

[optional] What gif best describes this PR or how it makes you feel?

- Implement automatic image collection from multiple source locations - Add path fixing for included markdown files (source and doctree level) - Use builder-inited event for reliable image copying timing - Add build output copying to ensure images are available in HTML - Fix HTML paths to use relative paths for local file viewing - Remove debug code and update README with image handling instructions This fix ensures images work correctly on both branch and main RTD builds, as well as local builds. The solution is generic and works for any contributor adding images anywhere in the documentation. Tested and verified on both test branch and main branch RTD builds.

aboada

I have reviewed conf.py. Some of the comments apply to other parts of the code.

Please let me know if you have any questions or need any help. Thanks!

aboada · 2025-12-18T03:32:09Z

doc/sphinx/source/conf.py

+    # Source locations to copy from
+    image_sources = [
+        # Main image directory (for OSE_ORGANIZATION.md and similar)
+        repo_root / "doc" / "assets" / "img",


Should we check the validity of repo_root before use or handle any exceptions?

aboada · 2025-12-18T03:38:03Z

doc/sphinx/source/conf.py

                try:
                    shutil.copy2(img, dest_file)
+                    copied_count += 1
                except Exception as e:


Should handle specific errors here. Some examples: PermissionError, FileNotFoundError. It applies to other exceptions in this file.

aboada · 2025-12-18T03:41:05Z

doc/sphinx/source/conf.py

                except Exception as e:
-                    print(f"Warning: Could not copy {img.name}: {e}")
+                    # Don't warn on overwrites (same file)
+                    if "already exists" not in str(e).lower():


Isn't this the same as:

exception shutil.SameFileError
This exception is raised if source and destination in copyfile() are the same file.

from shutil-documentation.

aboada · 2025-12-18T03:53:19Z

doc/sphinx/source/conf.py

+            figures_dir = source_file.parent / "figures"
+            if figures_dir.exists():
+                # Copy to _images (they'll be found there)
+                for pattern in ["*.png", "*.jpg", "*.jpeg", "*.gif", "*.svg"]:


It seems like we are processing images every time we find a markdown file. My suggestion for improving this is to keep track of the images (a set may do the job) and process them once.

aboada · 2025-12-18T03:56:03Z

doc/sphinx/source/conf.py

+    2. Standalone files with relative paths also benefit from the fix
+    3. The logic is safe - it only changes relative paths and skips absolute paths/URLs
+    """
+    import re


I suggest adhering to PEP8 guidelines.

From PEP8:

Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.
Imports should be grouped in the following order:

Standard library imports.
Related third party imports.
Local application/library specific imports.
You should put a blank line between each group of imports.

aboada · 2025-12-18T04:00:29Z

doc/sphinx/source/conf.py

+                            pass  # Ignore overwrites
+
+
+def fix_included_image_paths_source(app, docname, source):


Some of the functions in this file are too long and may be against the Single Responsibility Principle. I suggest keeping them short, concise, and focusing on a single concern (when possible). Just a comment for the future and something to keep in mind, it does not need to be addressed here.

aboada · 2025-12-18T04:14:02Z

doc/sphinx/source/conf.py

+
+    import shutil
+    from pathlib import Path
+    import os


Unused import.

aboada · 2025-12-18T04:14:56Z

doc/sphinx/source/conf.py

+                            pass  # Ignore overwrites
+
+
+def fix_included_image_paths_source(app, docname, source):


I think app and docname are not used within this scope. Please check.

aboada · 2025-12-18T04:22:49Z

doc/sphinx/source/conf.py

+        return fixed_path
+
+    # Match markdown image syntax: ![alt](path)
+    image_pattern = r'!\[([^\]]*)\]\(([^)]+)\)'


Does this regex consider a line with 2 or more images? What if the image has a title like:
![alt](path "Some Title Here")

See markdown-guide.

jbrzensk · 2025-12-22T21:34:15Z

@pritkc I checked the code, and it seems to function correctly. You only need to address the comments from @aboada, to ensure that if there are errors, they are handled correctly. Then we can modify the backward_euler image to be in the correct directory.

pritkc requested a review from valeriabarra December 17, 2025 09:40

pritkc self-assigned this Dec 17, 2025

pritkc mentioned this pull request Dec 17, 2025

Correction to Backward Euler C++ Example and Documentation #249

Merged

10 tasks

aboada reviewed Dec 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix image handling for RTD builds #260

Fix image handling for RTD builds #260

pritkc commented Dec 17, 2025

Uh oh!

aboada left a comment

Uh oh!

aboada Dec 18, 2025

Uh oh!

aboada Dec 18, 2025

Uh oh!

aboada Dec 18, 2025

Uh oh!

aboada Dec 18, 2025

Uh oh!

aboada Dec 18, 2025

Uh oh!

aboada Dec 18, 2025

Uh oh!

aboada Dec 18, 2025

Uh oh!

aboada Dec 18, 2025

Uh oh!

aboada Dec 18, 2025

Uh oh!

jbrzensk commented Dec 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		pass # Ignore overwrites


		def fix_included_image_paths_source(app, docname, source):

Fix image handling for RTD builds #260

Are you sure you want to change the base?

Fix image handling for RTD builds #260

Conversation

pritkc commented Dec 17, 2025

What type of PR is this? (check all applicable)

Description

Related Issues & Documents

QA Instructions, Screenshots, Recordings

Added/updated tests?

Read Contributing Guide and Code of Conduct

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

Uh oh!

aboada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbrzensk commented Dec 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants