Allow force polling when using reload with watchfiles #1580
Conversation
|
Hi @austinorr 👋 Can't you just not install |
Well, ya, I guess I could uninstall watchfiles after installing uvicorn[standard] but I don’t know what kind of reload experience to expect. Watchgod is deprecated so I don’t want to depend on that, and if the built in statreloader is just as good then maybe we don’t even need watchfiles in [standard] at all? |
|
We can add a marker on the standard extras for watchfiles on Windows (I didn't check the links you pasted above, so this is an idea, not a suggestion...). On the same line of thought, you can also not use the standard and install the dependencies manually. This is actually what I do personally... 👀 It would be cool to understand the difference between using statreloader and this flag on watchfiles in terms of performance. We can revisit the standard extra, but it would be outside this PR. 🙏 |
|
I actually do want to and need to use watchfiles with uvicorn and ‘just don’t install watchfiles’ does not leave me with a feature-complete alternative file watcher for ‘—reload’ mode during development. The behavior I need is fully supported by watchfiles, it’s just not accessible while running unicorn without this PR to pass the flag. |
|
I think this can be closed samuelcolvin/watchfiles#170. |
|
Thanks @samuelcolvin 🙏 @austinorr I'll be closing this then (hope is fine by you). If you want to discuss something else, feel free to create a discussion about it. |
|
@Kludex I still think this PR deserves at least a review and maybe even a merge, or at least a clear reason why it should be impossible to pass important arguments to I'm just not seeing a good reason this flag wasn't included in the 0.18.x release that dropped |
|
Not my decision but I would argue against merging this PR:
1. More PRs to review, more time, more important changes get delayed
2. More flags, more cognitive overhead, more scrolling, uvicorn becomes
harder to use.
3. In a stable and popular package you should need a good reason to review
and merge a PR, not a good reason to close a PR.
4. The discoverability point is null, you can add the new env variable to
uvicorn docs with no code changes
5. Much better to add more configuration settings via env variables to
watchfiles so they can be used by all the libraries using watchfiles, now
and in the future, with no future code changes.
…On Thu, 21 Jul 2022, 18:16 Austin Orr, ***@***.***> wrote:
@Kludex <https://github.com/Kludex> I still think this PR deserves at
least a review and maybe even a merge, or at least a clear reason why it
should be impossible to pass important arguments to watchfiles that are
related to its core functionality and system compatibility. A lot of users
likely get exposed to watchfiles only through uvicorn. Plus the fastapi
-> starlette -> uvicorn ecosystem is pulling in new users all the time,
so you'll probably get this issue again and again. This minimal, tested,
and fully covered diff nips all that in the bud and enables the workaround
and documents it in the uvicorn docs (e.g., "having trouble with reload
on wsl? Try using the --reload-force-polling flag as a workaround"). This
would be much more helpful than forcing users to discover this hidden env
flag in watchfiles (for which I am grateful, btw @samuelcolvin
<https://github.com/samuelcolvin> ) while trying to understand why their uvicorn
--reload isn't working after updating from 0.17.x to 0.18.x.
I'm just not seeing a good reason this flag wasn't included in the 0.18.x
release that dropped watchgod, and I see this diff as a pretty minimal
patch on that release.
—
Reply to this email directly, view it on GitHub
<#1580 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA62GGI3FF4SCSJE6D35GVLVVGAV7ANCNFSM54FSBPIA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
What @samuelcolvin said 🙏 PR welcome to add a note about the env variable with the reason for use it. |
|
Sounds good, I will open a new PR pinning watchfiles>=0.16.0 and adding a brief aside about the existence of the env variable to the uvicorn docs. @Kludex @samuelcolvin Thanks for your time and effort! |
Recent release prioritizes the excellent
watchfilesfile-system notifications based library over the now-deprecatedwatchgodpolling based library. This is overall an excellent development, but it leaves some windows developers who rely on containers out in the cold, since filesystem notifications aren't reliably passed through from windows to linux (see docker for windows issue, and workaround tldr: "don't do that, just use WSL's filesystem and everything works")This PR offers a bridge and escape hatch to allow for some transition time while we migrate our projects from blended file systems (windows + WSL, a natural starting point for many developers dealing with employer-provided windows hardware) to a WSL-only project filesystem format. I promise to start new projects as WSL only, but for now, allowing for polling rather than system notifications seems to work on blended setups.
The only change this PR proposes is to add a command line flag to allow a developer like me to opt in to the new-default-hotness
watchfileslibrary while still running the polling variant of the watcher on existing projects.The proposed API follows along with conventions established by other reload kwargs, and appears in usage like so:
Throughout the source code diff the new variable is called
reload_force_pollingand is entirely optional (default=False).For testing and coverage I've started this draft PR by modifying existing tests only. Thus far I've modified the test to make sure that the reloader initializes fully and the test to make sure that the polling side of the
watchfilesapi was used and successful for a changed .py file.The test patterns in this diff could be easily extended to other tests if needed, though this library may not need to extensively test all permutations of the
watchfilespolling switch. It seemed more important to just ensure that the flag is honored when its passed. That said, I'm happy to add the modification to other tests if desired.Thanks for the maintenance, docs, and frequent releases. Please consider this minor enhancement for merging - or let me know what I need to add/change to get it ready.