NotificationsYou must be signed in to change notification settings
Fork821
Star9.5k

Don't include`cwd()` when non-empty`--reload-dirs` is passed#2598

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Kludex merged 5 commits intoencode:masterfromstinovlas:fix-reload-dir

Apr 19, 2025

Merged

Don't include`cwd()` when non-empty`--reload-dirs` is passed#2598

Kludex merged 5 commits intoencode:masterfromstinovlas:fix-reload-dir

Apr 19, 2025

Conversation

Copy link

Contributor

stinovlas commentedMar 14, 2025

Summary

This PR fixes the behavior of--reload-dir to match the documentation. The documentation states:

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

also:

--reload-dir <path> - Specify which directories to watch for python file changes. May be used multiple times. If unused, then by default the whole current directory will be watched. If you are running programmatically usereload_dirs=[] and pass a list of strings.

However, the current implementation always addsPath.cwd() to list of watched directories. This is undesirable and contradicts the documentation.Path.cwd() should only be added as a default when no--reload-dir is specified.

Checklist

I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
No documentation update is required.

stinovlas added2 commits

March 14, 2025 13:08

Fix reload dir behavior

38100e8

fixup! Fix reload dir behavior

1e4d165

Ale-Cas reviewed

Mar 18, 2025

View reviewed changes

uvicorn/supervisors/watchfilesreload.py

Comment on lines -66 to -69

		if Path.cwd() not in directory.parents:
		self.reload_dirs.append(directory)
		if Path.cwd() not in self.reload_dirs:
		self.reload_dirs.append(Path.cwd())

Copy link

Ale-CasMar 18, 2025

Choose a reason for hiding this comment

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

I think that in order to correctly match the documentation you still need to add the cwd when config.reload_dirs is empty, so the code that you removed can be replaced with:

ifnotconfig.reload_dirs:self.reload_dirs.append(Path.cwd())

Copy link

ContributorAuthor

stinovlasMar 21, 2025

Choose a reason for hiding this comment

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

That's what I had there earlier, but it seems thatPath.cwd() is added somewhere else in the chain, this function never actually receives emptyconfig.reload_dirs, which is why I wasn't able to test this in any sensible way.

Copy link

Ale-CasMar 21, 2025

Choose a reason for hiding this comment

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

You're right, it's probably just added when initializing the config in that case. As a minor thing, I would still change this code a little bit to usePath.cwd() instead ofPath(os.getcwd()) so that it's also easier to search where that is added in the chain.

Copy link

ContributorAuthor

stinovlasMar 27, 2025

Choose a reason for hiding this comment

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

Sure, I wanted to keep the change minimal and isolated, but changingPath(os.getcwd()) toPath.cwd() won't hurt anything, so I applied the suggestion.

Copy link

adhami3310 commentedMar 21, 2025

i tried doing so in#2583 but it seems the maintainer has better things to do :/ since then i moved to a different library that handled reload dir in a sensible way

Copy link

Member

Kludex commentedMar 21, 2025

i tried doing so in#2583 but it seems the maintainer has better things to do :/ since then i moved to a different library that handled reload dir in a sensible way

Pretty rude considering your company leverages the contributions I've made to the ecosystem.

Copy link

adhami3310 commentedMar 21, 2025

Pretty rude considering your company leverages the contributions I've made to the ecosystem.

Everyone appreciates your contributions! We're also trying to contribute back :)

If you're busy with other things, that's ok. Everyone's been there, I've been there. I think, however, one shouldaim to communicate with their community on the timescale they expect things to happen. If the community finds an issue in the project they want to fix, and they offer the fix to you, it's (I'm not going to say common) decency to acknowledge them. In the same way you expect people to appreciate your contributions make them feel like their contributions are appreciated.

I talk in ideas of course, who among us is perfect.

fixup! fixup! Fix reload dir behavior

15076d2

stinovlas requested a review fromAle-Cas

April 7, 2025 08:28

Kludex changed the title~~Fix reload dir behavior~~Don't includecwd() when non-empty--reload-dirs is passed

Apr 13, 2025

Kludex reviewed

Apr 13, 2025

View reviewed changes

tests/supervisors/test_reload.py OutdatedShow resolvedHide resolved

Update tests/supervisors/test_reload.py

1f58dd6

Copy link

Member

Kludex commentedApr 13, 2025•
edited
Loading

Hmmm... What about the scenario when you runuvicorn main:app --reload --reload-dirs jinja-templates?

If you change themain.py, it will not reload. I understand that we don't wantPath.cwd(), but maybe we want the module's app directory to be included? e.g. onapp.main:app, we'd watch what is insideapp.

I can see how confusing it can be for people that, for example, just include a folder with jinja files into--reload-dirs.

Kludex assignedstinovlas

Apr 13, 2025

Copy link

ContributorAuthor

stinovlas commentedApr 14, 2025

Hmmm... What about the scenario when you runuvicorn main:app --reload --reload-dirs jinja-templates?
If you change themain.py, it will not reload. I understand that we don't wantPath.cwd(), but maybe we want the module's app directory to be included? e.g. onapp.main:app, we'd watch what is insideapp.
I can see how confusing it can be for people that, for example, just include a folder with jinja files into--reload-dirs.

If it were up to me, I wouldn't automagically add anything to the watchlist. If you specify--reload-dirs, you're already overriding the default behavior. IMHO, explicit is better than implicit.

Copy link

ContributorAuthor

stinovlas commentedApr 16, 2025

One more point is that I tried to make this PR as small as possible and not introduce any behavior changes apart from fixing the implementation to match the docs. I can see some arguments for adding the application dir (as well as some arguments against it), but I think that adding the application dir should be out of scope of this particular PR.