Skip to content

Local development: celery/build container don't restart on files changes #8802

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.

Sign up for GitHub

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

Jump to bottom
Closed
humitos opened this issue Jan 10, 2022 · 3 comments · Fixed by #9338
Closed

Local development: celery/build container don't restart on files changes #8802

humitos opened this issue Jan 10, 2022 · 3 comments · Fixed by #9338
Labels
Accepted Accepted issue on our roadmap Bug A bug Good First Issue Good for new contributors Sprintable Small enough to sprint on

Comments

@humitos
Copy link
Member

humitos commented Jan 10, 2022

We are using watchmedo to listen to file changes and restart celery and build containers. However, this has stopped working for some reason. If you change any Python file you will notice that these containers are not restarted and you have to manually execute inv docker.restart "celery build" to load the changes, which takes more time to restart than with watchmedo.

The watchmedo command executed is at https://github.com/readthedocs/common/blob/4d31f030bde728e2a86ccafbaceadca17122ae15/dockerfiles/entrypoints/build.sh#L12-L20 and https://github.com/readthedocs/common/blob/4d31f030bde728e2a86ccafbaceadca17122ae15/dockerfiles/entrypoints/celery.sh#L14-L23
@humitos humitos added Bug A bug Accepted Accepted issue on our roadmap labels Jan 10, 2022
@humitos humitos added Good First Issue Good for new contributors Sprintable Small enough to sprint on labels Mar 2, 2022
@alella
Copy link

alella commented Apr 7, 2022

This is due to a bug in watchdog library. A quick fix would be to remove the --kill-after=5 flag and let it default to 10 seconds.

Traceback from watchmedo

  File "/usr/local/bin/watchmedo", line 8, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.8/dist-packages/watchdog/watchmedo.py", line 645, in main
    args.func(args)
  File "/usr/local/lib/python3.8/dist-packages/watchdog/watchmedo.py", line 636, in auto_restart
    handler.stop()
  File "/usr/local/lib/python3.8/dist-packages/watchdog/tricks/__init__.py", line 192, in stop
    kill_time = time.time() + self.kill_after
TypeError: unsupported operand type(s) for +: 'float' and 'str'

@humitos
Copy link
Member Author

humitos commented Apr 7, 2022

@alella thanks a lot for sharing this! I opened a PR at readthedocs/common#106 to fix this issue. I tested it locally and it worked fine.

@humitos
Copy link
Member Author

humitos commented Apr 12, 2022

I'm hitting these issues now, and I'm not sure how to exclude .tox directory for example, and others

We could replace watchmedo by watchman, but that will require some extra effort: https://facebook.github.io/watchman/docs/watchman-make.html

humitos added a commit that referenced this issue Jun 15, 2022
@humitos
Local development: use nodemon to watch files instead of watchmedo
9a2d20f 
We have been dealing with a problem with `watchmedo` that restart the process
when it shouldn't be restarted and it doesn't restart it when it should.

I got tired debugging `watchmedo` and I was suggested to give it a try to
`nodemon` because we are already using it in other projects. I'm not super happy
adding a node dependency to the Dockerfile, but I didn't find a better way to do it.

I did some tests with the configuration proposed and it seemed to work pretty
well. I'm sure we will find some edge cases, but we always can tune it a little
more later.

Closes #8802
@humitos humitos mentioned this issue Jun 15, 2022
Merged
humitos added a commit that referenced this issue Jun 16, 2022
@humitos
Local development: use nodemon to watch files instead of watchmedo (
97ee9ba 
#9338)

* Local development: use `nodemon` to watch files instead of `watchmedo`

We have been dealing with a problem with `watchmedo` that restart the process
when it shouldn't be restarted and it doesn't restart it when it should.

I got tired debugging `watchmedo` and I was suggested to give it a try to
`nodemon` because we are already using it in other projects. I'm not super happy
adding a node dependency to the Dockerfile, but I didn't find a better way to do it.

I did some tests with the configuration proposed and it seemed to work pretty
well. I'm sure we will find some edge cases, but we always can tune it a little
more later.

Closes #8802

* Local development: remove `watchdo` Python package

It's not required anymore since we are using `nodemon` now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Accepted Accepted issue on our roadmap Bug A bug Good First Issue Good for new contributors Sprintable Small enough to sprint on
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Local development: use nodemon to watch files instead of watchmedo readthedocs/readthedocs.org
2 participants
@humitos @alella