Skip to content
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

Supporting glob-style operations #123

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

choldgraf
Copy link

@choldgraf choldgraf commented Feb 9, 2023

OK this one has bugged me for long enough that I decided to try and actually implement a fix.

This PR adds support for glob-module-style pattern matching, so you should be able to do --ignore **/*ignore*.txt or --ignore **/ignore and it should just work.

I added a test that I think checks the edge-cases here, but welcome to have thoughts on how to improve.

maybe @asmeurer or @consideRatio would be willing to give a look? I think both of you have gotten bitten by this one before as well :-)

Note that I also ran pre-commit autoupdate and ran it on all our files, because I think an outdated version was causing pre-commit to fail.

@welcome
Copy link

welcome bot commented Feb 9, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

Copy link
Contributor

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM!

I have a gut feeling that the larger ignore logic where this is a part could be refactored and reduce in complexity, but I think that is out of scope for this PR and especially not something to address now if I can't propose something concrete.

noxfile.py Outdated Show resolved Hide resolved
Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
@asmeurer
Copy link

asmeurer commented Feb 10, 2023

I'm not currently using autosummary, so it's not easy for me to test that use-case.

However, I have had an issue with sphinx-autobuild ignores in general, which may or may not be related. In the SymPy docs, we have a lot of plots that are generated by the matplotlib directive. Sometimes, sphinx-autobuild gets stuck in a loop thinking that every single one of these files is updated, like

[sphinx-autobuild] Detected change: /Users/aaronmeurer/Documents/Python/sympy/sympy/doc/_build/plot_directive/modules/plotting-35.pdf

It does this one at a time for every image in the build, for which we have over 100.

This happens even if I do --ignore=_build/ (I tested it and it happens in this branch too).

If you want to reproduce this in the SymPy docs, you can install the dependencies from doc/requirements.txt then, from the doc/ directory run

make livehtml # Runs sphinx-autobuild

then, after it is finished the initial build, in another terminal run

make clean

(the docs take a couple of minutes to build from scratch)

The infinite loop honestly wouldn't be so bad, except the webserver gets blocked by it, meaning if you try to click a link in the live webpage, it tries to wait for the build to finish before it loads the page, and thus never loads. Making the webserver not block like this in general would be helpful.

Happy to open a separate issue for this if you'd like.

@asmeurer
Copy link

This sort of looping also happens whenever I checkout a new branch, one at a time for every file in the source repo that was changed. So I guess there are three problems here:

  • sphinx-autobuild is not ignoring _build/, even when told to do so with --ignore
  • The webserver blocks while a build is happening. This is particularly annoying when this looping happens, but it's also annoying in general. Could it be made asynchronous?
  • When a bunch of files change, it does a bunch of rebuilds, one for each file. It ought to be able to detect changes in a batch to avoid unnecessary duplicate rebuilds.

@choldgraf
Copy link
Author

choldgraf commented Feb 12, 2023

Hmm - I'm not sure why your build folder isn't being ignored ..indeed I would have expected this PR to fix it. I can try to take another look and see if i may have missed something.

For the async and batch blocking stuff unfortunately i have no idea how that stuff works, and also have no real knowledge of this codebase other than what I learned while making this PR, so i think we should just open an issue to track it over time. (We also need to find a way to make a release, this repository got moved into executable books without transferring the ability to upload to pypi)

FWIW I'd be more than happy to give you merge rights if you wanted to give a shot at fixing any of this! This repository basically has no maintainers right now

@choldgraf
Copy link
Author

Another quick thought: can you double check that the path to your _build folder is correct when you pass the --ignore argument? I didn't see --ignore in your makefile. What was the specific call that you use when using --ignore?

I opened two issues to track two of your requests above:

For the "_build" folder issue, I couldn't replicate it locally in a toy site I put together. I made a little Sphinx site with this structure:

  • docs
  • build
  • And a noxfile.py to run sphinx-autobuild

I then set conf.py in docs to generate a folder with 10 files in it in build/tmp at build time.

I then used this invocation of sphinx-autobuild:

sphinx-autobuild -b html docs/ build/html --ignore build --port 0

and it correctly ignored the build folder. So I'm not sure why you aren't getting that behavior.

@asmeurer
Copy link

Yes, I never actually committed --ignore to the sympy Makefile because it never seemed to actually work. If you're testing this, add --ignore=_build/ (and anyway, shouldn't the build folder be ignored automatically?). Here's the full line I am using to test this in the SymPy Makefile

sphinx-autobuild --open-browser --ignore=_build/ --watch .. --host $(LIVEHOST) --port $(LIVEPORT) -b html $(ALLSPHINXOPTS) $(SOURCEDIR) $(BUILDDIR)/html

@choldgraf
Copy link
Author

I spent a while trying to get the sympy docs to build this morning but could never figure it out. I got a string of error messages that I had to resolve by installing certain system libraries etc, but the last one I ran into I didn't know how to debug AttributeError: module 'unicodedata2' has no attribute 'ucnhash_CAPI' and I ran out of time to work on this.

I am not sure why this doesn't work for you, but I think this PR at least solves some of the edge-cases that have been mentioned. I can remove the "closes" for your glob issue in case that is not resolved, but I'd suggest that we merge this one as an iterative improvement so that we can get it in before I go on paternity leave in a few days.

@asmeurer
Copy link

I've seen that error before. I'll have to see if I can remember what causes it.

@asmeurer
Copy link

And anyway, feel free to just open an issue for my problem. It's already a problem for me and this PR didn't seem to make it worse. Although I admit I didn't actually test the actual globbing nature of this PR.

@asmeurer
Copy link

If it's not a problem could you open a SymPy issue with the problem you are seeing?

@guideloom
Copy link

What are the chances this getting approved? This is much needed! :)

@choldgraf
Copy link
Author

Sorry - I had a baby and dropped off the face of the earth. I can try to get back to this at some point but am on paternity leave for the time being

@guideloom
Copy link

Sorry - I had a baby and dropped off the face of the earth. I can try to get back to this at some point but am on paternity leave for the time being

Congrats!!!

@asmeurer
Copy link

By the way, the repeated rebuilding issue doesn't just happen on SymPy. It seems to happen on any repo that uses autodoc with lots of Python files. Here's a simpler repo where I've also seen the issue that you can test https://github.com/quansight-labs/ndindex.

@tony
Copy link

tony commented Dec 23, 2023

@choldgraf Can you give this a rebase?

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