Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

os.getcwd() as default for reload_dirs is flaky #1289

Closed
2 tasks done
HansBrende opened this issue Dec 10, 2021 · 2 comments
Closed
2 tasks done

os.getcwd() as default for reload_dirs is flaky #1289

HansBrende opened this issue Dec 10, 2021 · 2 comments

Comments

@HansBrende
Copy link
Contributor

HansBrende commented Dec 10, 2021

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!

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.)

Debugging material

No response

Environment

Running uvicorn 0.16.0 with CPython 3.8.10 on Darwin

Additional context

No response

@euri10
Copy link
Member

euri10 commented Feb 16, 2022

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

@euri10 euri10 closed this as completed Feb 16, 2022
@HansBrende
Copy link
Contributor Author

@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 euri10 reopened this Feb 16, 2022
@encode encode locked and limited conversation to collaborators Feb 16, 2022
@euri10 euri10 converted this issue into discussion #1382 Feb 16, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants