os.getcwd() as default for reload_dirs is flaky

[HansBrende]

Checklist

• The bug is reproducible against the latest release or master.
• There are no similar issues or pull requests to fix it yet.

Describe the bug

IF your current working directory doesn't happen to match the app directory (which is often the case in IDEs such as intellij, where the current working directory by default is set to the directory containing the python file you are running, not the directory containing the associated python package, e.g. the src directory... OR if you've simply invoked your python script from a different directory than the one it's in....) then the default of os.getcwd() for reload_dirs makes no sense.

There is a "smarter" implementation to be had here, in 2 possible ways:

Case 1) User supplies app_dir: In this case, the default reload_dirs could simply be app_dir! (I would tend to avoid this approach in favor of Case (2) which works for all cases.)

Case 2) User does OR does not supply app_dir:

IF we are reloading, then we know app must be a string containing the fully qualified module name of the app! We can now look up the module path! By walking back the module path to the beginning of the fully qualified module name, we can find the source directory which is what we should be watching for source changes by default!

Here is an example (simplified) implementation for case 2. If this idea is pleasing, I will turn it into a PR with more bells and whistles.

if reload and not reload_dirs:            
    spec = importlib.util.find_spec(app[:app.index(':')])  # find but do not _load_ the app module!
    module_components = spec.name.split('.')
    src_path, fname = os.path.split(spec.origin)
    if fname != '__init__.py':
        src_path = spec.origin
    for _ in module_components:
        src_path = os.path.dirname(src_path)
    reload_dirs = [src_path]

Steps to reproduce the bug

With the following app structure:

{project_dir}/src/main/main_app/app.py  <- location of app
{project_dir}/src/main/main_util.py
{project_dir}/src/ext/ext_util.py

and cwd set to src/main/main_app (or to some sibling of {project_dir}),

run:

uvicorn.run('main.main_app.app:app', app_dir='{project_dir}/src', reload=True)

Now make a change to the file main_util.py.
Now make a change to the file ext_util.py.

Expected behavior

I personally would expect the app to reload after either main_util.py was changed or ext_util.py was changed, regardless of where your cwd is.

Actual behavior

The app reloads after neither change. (And though the current behavior is documented, it is both suboptimal and surprising IMO, and easily fixed by my proposed tweak.)

Environment

Running uvicorn 0.16.0 with CPython 3.8.10 on Darwin

Additional context

ADDENDUM: I see by the mention in the original issue that I am not the only one who feels cwd as a default is insufficiently robust 😅

[euri10]

use --reload-dirs or there's something I'm missing ?

[HansBrende]

@euri10 yes, I believe you are missing something: I'm saying that the default logic can be improved if no --reload-dirs arg is supplied. Again... I am happy to implement a PR for this myself. Perhaps can this issue be re-opened for further discussion if you believe that the existing default logic cannot be improved upon and is perfect the way it is? I'd like to understand better. CWD is (often) the same directory that your app is located in, but the two have nothing to do with each other in general so I'm a bit confused why you wouldn't want to use the actual directory containing the app module as the default???

[euri10]

ok let's see what other think of this
in my case, as you mentioned IDE, I use pycharm and use their Working directory config while debugging so I never felt this default was bad.

[HansBrende]

Yep, I use IntelliJ w/Python plugin which (I believe) has the same behavior as PyCharm. The issue occurs when your src directory is slightly more complex than just one top-level app (e.g. if your app is in a subdirectory rather than the top-level package). IntelliJ by default will set CWD to the folder containing the file you clicked "run" in. In which case... all the files outside that folder are ignored. So it'd be super nice if we didn't have to specify --reload-dirs every time (or manually override IntelliJ's default CWD every time), when simply supplying the module name gives you all the information needed to locate the root folder.

[HansBrende]

ALSO: It is much nicer for README's to not have to go into all the "make sure you are executing this command from this directory", or, "make sure before running that you've set intellij's CWD to yada yada yada". Much nicer to say: just execute this command and it works.