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?

The repo_root is derived from app.confdir which is guaranteed by Sphinx to be valid during builds.
Path operations on it won't raise exceptions; they'll just return non-existent paths, which we already handle via img_source.exists() checks in the loop

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.

Fixed in commit 3a15829. Covered exception handling for shutil.SameFileError, PermissionError, and FileNotFoundError.

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.

You're right! Fixed in commit 3a15829

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.

Good suggestion! Currently, the code handles duplicate copies via the try/except block.
I'll note this as a future optimization since the current approach works correctly. Should I implement this optimization in this PR or defer it?

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.

Those function-level imports are for lazy loading. This pattern was common in Sphinx configurations where functions may not always be called. If you'd prefer I remove the redundant 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.

Acknowledged, thank you for the feedback. I'll keep this in mind for future refactoring

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

doc/sphinx/source/conf.py

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


Unused import.

Fixed in commit 45151e2

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.

Edit - Fixed in commit 1b2a8d6. Prefixed them with underscore (_app, _docname) to indicate intentional non-use, and added docstring documentation explaining this

These parameters are required by Sphinx's source-read event signature (app.connect('source-read', fix_included_image_paths_source)). Sphinx passes these arguments automatically, so we must accept them in the function signature even if not used.
See Sphinx source-read event docs.

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.

Good question!

Yes, re.sub replaces all matches globally

The current pattern [^)]+ matches greedily up to the first ), so ![alt](path "Title") would match path "Title" as the path. When we extract the filename with Path(img_path).name, this would result in "Title") which is incorrect. However, after re-reviewing our documentation source files, we don't use image titles anywhere. Should I add support for this edge case for future-proofing, or document it as a known limitation?

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.

Addresses review comment: 'Unused import.' Removed unused 'import os' statements from: - fix_included_image_paths_source() - fix_included_image_paths_doctree() - copy_images_to_build_output()

Addresses review comments: - 'Should handle specific errors here. Examples: PermissionError, FileNotFoundError' - 'Isn't this the same as shutil.SameFileError?' Changed exception handling in image copy operations: - Use shutil.SameFileError for same-file scenarios (silently skip) - Catch PermissionError and FileNotFoundError specifically - Removed string matching on error messages

Addresses review comment: 'I think app and docname are not used within this scope. Please check.' In fix_included_image_paths_source(), the 'app' and 'docname' parameters are required by Sphinx's source-read event signature but not used in the function body. Prefixed with underscore to indicate intentional non-use. Added docstring Args section to document this design decision.

pritkc · 2026-01-03T05:35:15Z

Thanks, @aboada, for the detailed feedback! I’ve worked through most of the points and would appreciate you taking another look. I’d also love your thoughts on a few questions I still have.

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

pritkc added 4 commits January 2, 2026 19:06

fix: remove unused os imports in image handling functions

45151e2

Addresses review comment: 'Unused import.' Removed unused 'import os' statements from: - fix_included_image_paths_source() - fix_included_image_paths_doctree() - copy_images_to_build_output()

Merge branch 'main' into test/rtd-image-fix-fork-test-2.0

8a753e2

pritkc requested a review from aboada January 3, 2026 05:33

		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

Uh oh!

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!

pritkc Jan 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

pritkc Jan 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pritkc commented Jan 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pritkc Jan 3, 2026 •

edited

Loading

pritkc Jan 3, 2026 •

edited

Loading