Skip to content

Check modification times of included RST files #20374

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

Merged
merged 4 commits into from
Jul 7, 2021
Merged

Check modification times of included RST files #20374

merged 4 commits into from
Jul 7, 2021

Conversation

yongrenjie
Copy link
Contributor

@yongrenjie yongrenjie commented Jun 6, 2021

Fixes #17860. Briefly, to reproduce this bug:

  1. Create a file included.rst with a plot inside it.

  2. Create a file index.rst containing the include directive
    .. include:: included.rst.

  3. Build Sphinx documentation.

  4. Modify the plotting code inside included.rst, but don't touch
    index.rst.

  5. Rebuild the Sphinx documentation. Because index.rst wasn't touched,
    plot_directive thinks that it's up-to-date and thus the image doesn't
    need to be re-created.

This commit fixes the issue by checking the modification times not only
of the source file (index.rst in this case), but also of the file(s)
included within it (included.rst).

PR Summary

The commit message contains most of the info, but I attach some extra detail here just in case people come across it and find it useful. There are a couple of ways to get the included files:

Method 1

Since docutils 0.17, there is an attribute state.document.include_log which is a list of tuples containing the names of any files that were included, which is exactly what we want.

[('index.rst', (None, None, None, None), 4.611686018427388e+18),
 ('included.rst', (None, None, None, None), 21)]

Method 2

Directly inspect state_machine.input_lines.items, which is a list of tuples (source, offset). If a RST file is included, then we can get its filename (relative to the top-level Sphinx directory) in source. For example, this is what plot_directive can when I place .. include:: included.rst inside the file ~/test/plot_dir/index.rst:

[('included.rst', 2),
 ('included.rst', 3),
 ('included.rst', 4),
 ('included.rst', 5),
 ('included.rst', 6),
 ('included.rst', 7), 
 ('included.rst', 8),
 ('included.rst', 9),
 ('included.rst', 10),
 ('included.rst', 11),
 ('included.rst', 12),
 ('internal padding after included.rst', 13),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 7),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 8),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 9),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 10),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 11),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 12),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 13),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 14),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 15),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 16),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 17),
 ('/Users/yongrenjie/test/plot_dir/index.rst', 18)
]

The issue with this is that there's the 'internal padding after...' line which we would have to filter out, and I'm not sure what other directives (if any) might also add some junk to this list. I got around this by first removing duplicates and then checking which of the entries were valid files.

I feel like Method 2 is a bit fragile, so I made it as a fallback:

try:
    # method 1...
except AttributeError:    # indicates docutils <=0.16
    # method 2...

I've tested this all the way back to docutils 0.14 and it seems to be pretty reliable.

PR Checklist

  • N/A Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • N/A New features are documented, with examples if plot related.
  • N/A Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • N/A New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • N/A API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

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 while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. 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.

@tacaswell tacaswell added this to the v3.5.0 milestone Jun 7, 2021
@tacaswell
Copy link
Member

Thank you for your work on this! The approach looks sound to me, however I have a slight concern about the implementation.

How "public" are these functions? That is, do we expect anyone outside of the internal code to the plot extension to be using them? These changes introduce backwards-incompatible changes (because it adds positional arguement someplace in the middle). It would be safer from a back-compat stand point (but less great from a forward looking API point of view) to do the signatures as

def out_of_date(original, derived, includes=None):
    if includes is None:
        includes = []
    ...
def render_figures(code, code_pathoutput_dir, output_base, context,
                  function_name, config, context_reset=False,		                   
                   close_figs=False, code_includes=None):
    if code_includes is None:
        code_includes = []
    ...

so if anyone is using these we won't break them!

@yongrenjie
Copy link
Contributor Author

@tacaswell I didn't think about that possibility, to be honest. I assumed that the implementation of plot_directive was internal and that the user-facing part would be the Sphinx extension itself. However, I entirely agree with the backwards compatibility point, those functions are exposed and can be imported, so there might be someone out there using it. I'll modify it as per your suggestion.

Thanks for taking a look & commenting! 😄

@yongrenjie
Copy link
Contributor Author

@tacaswell I updated and rebased, let me know if there's anything else I could change.

@QuLogic
Copy link
Member

QuLogic commented Jun 7, 2021

I'm not sure if it does check this for non-included files anyway, but can you update the test for this?

@yongrenjie
Copy link
Contributor Author

@QuLogic As far as I could tell, tests/test_sphinxext.py only runs a single pass of python -m sphinx, so there isn't yet anything that tests plot regeneration based on modification time.

Got a bit of a busy time ahead, so I'm not sure I'll be able to add new tests for this right now, sorry—I wouldn't mind giving it a shot in the near-ish future though!

@yongrenjie yongrenjie marked this pull request as ready for review June 26, 2021 21:10
The new tests:
 - make sure that unchanged plots are not recreated upon a second
   invocation of sphinx-build
 - make sure that plots with `:context:` are recreated (#20523)
 - make sure that plots included via `.. include:` are updated when
   modified (#17860)
@yongrenjie
Copy link
Contributor Author

Sorry for the delay (real life got in the way); but I think this is ready for review. As a summary: I fixed the original bug (#17860), fixed an extra bug along the way (#20523, there's rather more explanation there), and added tests to make sure both behave correctly. PEP8 tests are all fine.

Co-authored with @QuLogic

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@yongrenjie
Copy link
Contributor Author

Thanks @QuLogic for your corrections!

I think that the out_of_date function is still needed for two cases - (1) is when the plot comes from a python file .. plot:: my_plot.py and (2) for the included rst files (which is what I tried to fix). Unless I'm missing something, which might well be the case.

@QuLogic
Copy link
Member

QuLogic commented Jul 7, 2021

I just meant it could be inlined.

@QuLogic QuLogic merged commit a4253d5 into matplotlib:master Jul 7, 2021
@QuLogic
Copy link
Member

QuLogic commented Jul 7, 2021

Thanks @yongrenjie! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

@yongrenjie
Copy link
Contributor Author

yongrenjie commented Jul 7, 2021

Thank you! 😄 BTW I see what you meant now with inlining... I could put in another PR if you like.

def out_of_date(original, derived, includes=None):
    if not os.path.exists(derived):
        return True
    else:
        derived_mtime = os.stat(derived).st_mtime  # or maybe try/except
    if includes is None:
        includes = []
    files_to_check = [original, *includes]
    return any((derived_mtime < os.stat(f).st_mtime) for f in files_to_check)

@QuLogic
Copy link
Member

QuLogic commented Jul 7, 2021

Sure, feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plot_directive is confused by include directives, part 2 (context option) Plot directive may be confused by ..include::
5 participants