Skip to content
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

Daemon #497

Closed
wants to merge 3 commits into from
Closed

Daemon #497

wants to merge 3 commits into from

Conversation

jachym
Copy link
Member

@jachym jachym commented Oct 10, 2019

This PR introduces new way of async process execution. Async processes are stored to database.

User also needs to start pywps_daemon.py script, which will be running as background process and check the database for incoming requests and execute them (using the processing.Job mechanism)

Besides that, some smaller changes are introduced:

  • Removing tests of Python 2
  • Removing async execution from Process class
  • Adding more pythonish class attribute setters for uuid
  • Adding Json representation of process to stored_processes
  • Adding dbglog tests

Contribution Agreement

(as per https://github.com/geopython/pywps/blob/master/CONTRIBUTING.rst#contributions-and-licensing)

  • I'd like to contribute [feature X|bugfix Y|docs|something else] to PyWPS. I confirm that my contributions to PyWPS will be compatible with the PyWPS license guidelines at the time of contribution.
  • I have already previously agreed to the PyWPS Contributions and Licensing Guidelines

* Adding more pythonish class attribute setters for uuid
* Adding Json representation of process to stored_processes
* Adding dbglog tests
@coveralls
Copy link

coveralls commented Oct 10, 2019

Coverage Status

Coverage increased (+1.7%) to 75.907% when pulling bde3551 on jachym:daemon into 4ea5571 on geopython:master.

@jachym
Copy link
Member Author

jachym commented Oct 16, 2019

Hm .. probably shell script will be needed for better implementation of the start stop and restart methods :-|

https://dpbl.wordpress.com/2017/02/12/a-tutorial-on-python-daemon/

Copy link
Collaborator

@cehbrecht cehbrecht left a comment

Choose a reason for hiding this comment

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

See inline comments and questions.

the next pending request.
"""
try:
from pywps.processing.job import Job, JobLauncher
Copy link
Collaborator

Choose a reason for hiding this comment

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

JobLauncher not used.

@cehbrecht
Copy link
Collaborator

cehbrecht commented Oct 22, 2019

Running a test with Emu WPS. sync requests work as expected. When running an async execute request I get the following error:

sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) table pywps_stored_requests has no column named process
[SQL: INSERT INTO pywps_stored_requests (uuid, request, process) VALUES (?, ?, ?)]

We are missing an upgrade mechanism of the database?

In twitcher we are using alembic.

@cehbrecht
Copy link
Collaborator

@jachym @huard @davidcaron
In birdhouse deployment we currently use Werkzeug to start the WPS service for development:
https://github.com/bird-house/cookiecutter-birdhouse/blob/7a1259731d1d532b719bcc3fcbe036578fc95fa7/%7B%7Bcookiecutter.project_repo_name%7D%7D/%7B%7Bcookiecutter.project_slug%7D%7D/cli.py#L134

In production mode we use supervisor deployed by Ansible:
https://github.com/bird-house/ansible-wps-playbook/blob/master/roles/pywps/templates/supervisor.conf.j2

When we merge this PR our installations will break! We need to update our service start scripts.

If we accept this PR it needs to go into a 4.4.0 release (together with dropping Python 2.x).

@cehbrecht
Copy link
Collaborator

@jachym
This PR with separation of the watchdog code to poll jobs from the database makes the code look cleaner. It might also make the service less error-prone.

Could the watchdog also be a cronjob? So we would just need to add a cronjob (configuration task) instead of running an additional service?

We probably need to fix the database upgrade.

@huard
Copy link
Collaborator

huard commented Oct 22, 2019

Question: Is this PR intended to fix #495 ? #448 ?

@davidcaron
Copy link
Contributor

@jachym @huard @davidcaron
In birdhouse deployment we currently use Werkzeug to start the WPS service for development:
https://github.com/bird-house/cookiecutter-birdhouse/blob/7a1259731d1d532b719bcc3fcbe036578fc95fa7/%7B%7Bcookiecutter.project_repo_name%7D%7D/%7B%7Bcookiecutter.project_slug%7D%7D/cli.py#L134

In production mode we use supervisor deployed by Ansible:
https://github.com/bird-house/ansible-wps-playbook/blob/master/roles/pywps/templates/supervisor.conf.j2

When we merge this PR our installations will break! We need to update our service start scripts.

Thank you for the head up.

My 2 cents:

I think having a separate process to launch jobs is a good idea and will make the queue more resilient. It's closer to something like a celery task queue.

We deploy in containers, so the daemon will be in its own container, and a separate entrypoint command for the daemon is much better than a cron job in this case (1 process = 1 container).

@cehbrecht
Copy link
Collaborator

Tested with several emu sleep process. It works when I start the watchdog ... it fills the queue and runs until all jobs are finished:
pywps_daemon.py watchdog

... but the daemon mode ends immediately:
pywps_daemon.py start

@cehbrecht
Copy link
Collaborator

@jachym @davidcaron

We deploy in containers, so the daemon will be in its own container, and a separate entrypoint command for the daemon is much better than a cron job in this case (1 process = 1 container).

Could we provide both, cron-job and daemon service? So we can support several deployment scenarios. I think I would feel safer with a cron-job in the ansible/supervisor scenario. For the docker/container and also for the Werkzeug/demo mode we might want a daemon service.

@davidcaron
Copy link
Contributor

Could we provide both, cron-job and daemon service? So we can support several deployment scenarios. I think I would feel safer with a cron-job in the ansible/supervisor scenario. For the docker/container and also for the Werkzeug/demo mode we might want a daemon service.

Sure! I didn't mean one or the other. I agree the cron-job would be better with ansible outside of containters.

@cehbrecht
Copy link
Collaborator

@jachym @davidcaron @huard I want to work on this "watchdog" PR the next days to fix the job queue handling. I will open a new PR for this ... it will include the code of this one. I want to move code to a new watchdog module and move the command-line interface of our birdhouse template to pywps:
bird-house/cookiecutter-birdhouse#52

@cehbrecht cehbrecht mentioned this pull request Dec 16, 2019
@jachym
Copy link
Member Author

jachym commented Dec 31, 2019

@cehbrecht shall we close this PR and continue in #505 (?)

@cehbrecht
Copy link
Collaborator

@cehbrecht shall we close this PR and continue in #505 (?)

Yes ... I'm closing this PR. Maybe we should have worked on dev branch.

@cehbrecht cehbrecht closed this Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants