Override default --reload-dir if passing sub-directories of cwd

[renan-r-santos]

This PR attempts to fix hot reload if a user passes a subdirectory of the current working directory using either --reload-dir or --reload-include.

The documentation explicitly says that those options, if passed, should override the current working directory.

  --reload-dir PATH               Set reload directories explicitly, instead
                                  of using the current working directory.

We can see here that indeed cwd acts as a default value. However, the constructor of WatchFilesReload changes that behavior by discarding config.reload_dirs if cwd is a parent of those directories.

I've changed test_should_not_reload_when_only_subdirectory_is_watched to add a case for when cwd is a parent of the watched directory. I've also adapted other tests to match the documentation about --reload-dir, see test_default_watch_dir and test_should_watch_separate_dirs.

[renan-r-santos]

Hi @Kludex, if you have some time, would you mind taking a look at this PR? If you think this isn't the correct solution I can close it. Tks a lot

[Kludex]

Would you mind creating an MRE for what it tries to solve? Those flags are really confusing to me.

[renan-r-santos]

Hi @Kludex, sorry for the delay in answering your comment. I rebased my PR on top of the current master and tests are still passing. Here is a minimal repro:

mkdir -p uvicorn_repro && cd $_
python3 -m venv .venv
. .venv/bin/activate
python -m pip install 'uvicorn[standard]==0.20.0' 'fastapi==0.88.0'
mkdir -p src/uvicorn_repro/routes
touch src/uvicorn_repro/{__init__,main}.py
touch src/uvicorn_repro/routes/{__init__,routes}.py

main.py

import uvicorn
from fastapi import FastAPI


app = FastAPI()


@app.get("/")
def index():
    return {"home": "index"}


if __name__ == "__main__":
    uvicorn.run("main:app")

routes.py

print("Hello world")

Start uvicorn using --reload-dir and change anything inside src/uvicorn_repro/main.py

> uvicorn --reload-dir src/uvicorn_repro/routes --reload src.uvicorn_repro.main:app
INFO:     Will watch for changes in these directories: ['/home/rrodrigues/Downloads/uvicorn_repro/src/uvicorn_repro/routes']
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     Started reloader process [25287] using WatchFiles
INFO:     Started server process [25289]
INFO:     Waiting for application startup.
INFO:     Application startup complete.
WARNING:  WatchFiles detected changes in 'src/uvicorn_repro/main.py'. Reloading...
INFO:     Shutting down
INFO:     Waiting for application shutdown.
INFO:     Application shutdown complete.
INFO:     Finished server process [25289]
INFO:     Started server process [25301]
INFO:     Waiting for application startup.
INFO:     Application startup complete.

Even though uvicorn tells us that only the routes directory is watched for changes, changes outside of that directory still trigger a reload. This PR proposes a fix to that behavior.

Please, let me know if you need anything else to get this merged. Thanks

[Kludex]

Please, let me know if you need anything else to get this merged. Thanks

I need the context of the PR that added this. It looks a conscious decision, so I don't think "fix" is the right word (I can be wrong here)... :thinking:

[renan-r-santos]

In my opinion, I think it depends on how we interpret this specific part of $ uvicorn --help

  --reload-dir PATH               Set reload directories explicitly, instead
                                  of using the current working directory.

*emphasis mine

If this is supposed to be the intended behavior, then I'd treat this PR as a fix.

My feeling is that watching for changes only in a subdirectory of the current working directory is kind of a common use case for the hot reload feature, and considering that this doesn't work as expected in the current main branch, I'd treat it as a fix. Let me know what your thoughts are. Thanks

[Kludex]

It's not important if it's a fix or not... It's a change in behavior anyway.

In any case, I asked what I needed on my previous message. 😅

[Kludex]

This is the PR that introduced this behavior: https://github.com/encode/uvicorn/pull/820.

[renan-r-santos]

Thanks for finding this, Kludex. I won't have time to dig deeper into it before the holidays, but I remember when I looked into this that the issue is specifically with the WatchFilesReload class. For instance, if you add the as_cwd context manager to the test test_should_not_reload_when_only_subdirectory_is_watched as I did in this PR without modifying the implementation of WatchFilesReload and WatchGodReload classes, the first one fails while the second one passes. When I checked the PR that added WatchFilesReload, I think its constructor was copied from WatchGodReload. But somehow, they behave differently in this particular case. I can dig deeper to understand why later but I just want to point out why this PR only touches WatchFilesReload.

[Andrew-Chen-Wang]

I agree with the original PR comment. When trying to run uvicorn.run(reload_dirs=["subdirectory"]) or uvicorn.run(reload_includes=["subdirectory"]), I would expect the behavior to exclusively watch that subdirectory but the real results are that all the subdirectories are still watched.

You'll get a log message from uvicorn stating:

Will watch for changes in these directories: ['/absolute/subdirectory']

But it'll still be watching all the other subdirectories under cwd.

I've tried several other iterations as well:

uvicorn.run(
        "app.main:fast",
        host=host,
        port=port,
        log_level=log_level,
        reload=reload,
        reload_includes=["app/*", "config/*", "manage.py", ".env"],
    )

uvicorn.run(
        "app.main:fast",
        host=host,
        port=port,
        log_level=log_level,
        reload=reload,
        reload_dirs=["app", "config"],
        reload_includes=["app/*.py", "config/*.py", "manage.py", ".env"],
        reload_excludes=["*.py"]
    )

this one tries to use glob negation, but Python doesn't recognize it.

uvicorn.run(
        "app.main:fast",
        host=host,
        port=port,
        log_level=log_level,
        reload=reload,
        reload_excludes=["[!app/]*.py", "[!config/]*.py"],
    )

uvicorn.run(
        "app.main:fast",
        host=host,
        port=port,
        log_level=log_level,
        reload=reload,
        reload_includes=["app/**/*.py", "config/**/*.py", "*/.env"],
        reload_excludes=["*.py"],
    )

I think the current behavior is not intuitive, and yes I still haven't figured out how to exclusively watch those two subdirectories app and config.

[kssion]

@Kludex It's already 2024, why can't we only monitor specified subdirectories?

[Kludex]

The logic around the include/exclude on reload is rather complex, that's why there were several PRs around it over time. Also, it requires a lot of my time to make sure I do a proper review. It's not only about the diff you see here, but the context.

In any case... Given the amount of work I do on open source, I don't like reading comments that question my work on the projects I maintain.

I'm locking this issue to avoid further spam. The reload include/exclude needs a bit of review - I'll get back to it when I have time.