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

Flask-Apscheduler #1109

Merged
merged 14 commits into from
Mar 30, 2022
Merged

Flask-Apscheduler #1109

merged 14 commits into from
Mar 30, 2022

Conversation

i-oden
Copy link
Member

@i-oden i-oden commented Mar 30, 2022

Same as #1108 but without the mess.

Change the scheduled tasks to use Flask-Apscheduler instead -- this means that the setup structure is like the migrations, database etc etc. (init_app(app))
Have also removed the scheduler stuff from the utils.py module, and now the scheduled tasks are in a new file, and set up with the help of decorators.

There may be some more config needed but we'll notice that as we add the tasks. Also, the first task here is just an example and something that @valyo has started on implementing, but since there are more than one that's working on cronjobs etc now, I want to merge this before anyone starts so that it all works.

The previous solution currently has issues with the app context.

  • Tests passing
  • Black formatting
  • [ - ] Migrations for any changes to the database schema
  • Rebase/merge the dev branch
  • Note in the CHANGELOG

@i-oden i-oden mentioned this pull request Mar 30, 2022
5 tasks
@i-oden i-oden requested review from valyo, talavis and aanil March 30, 2022 08:08
@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Merging #1109 (1e819ea) into dev (c371604) will increase coverage by 0.29%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##              dev    #1109      +/-   ##
==========================================
+ Coverage   85.74%   86.03%   +0.29%     
==========================================
  Files          28       29       +1     
  Lines        3241     3237       -4     
==========================================
+ Hits         2779     2785       +6     
+ Misses        462      452      -10     
Impacted Files Coverage Δ
dds_web/utils.py 78.57% <ø> (+5.20%) ⬆️
dds_web/web/user.py 80.76% <ø> (ø)
dds_web/scheduled_tasks.py 62.50% <62.50%> (ø)
dds_web/__init__.py 61.61% <100.00%> (+0.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c371604...1e819ea. Read the comment docs.

Copy link
Contributor

@aanil aanil left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@talavis talavis left a comment

Choose a reason for hiding this comment

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

I can't get it working in Gunicorn (which is used for production). I have a task that triggers correctly when running via flask (standard docker-compose), but it does not trigger when I run in Gunicorn. If we can't get it working in Gunicorn, I do not see a reason to use it at all.

@valyo
Copy link
Member

valyo commented Mar 30, 2022

I can't get it working in Gunicorn (which is used for production). I have a task that triggers correctly when running via flask (standard docker-compose), but it does not trigger when I run in Gunicorn. If we can't get it working in Gunicorn, I do not see a reason to use it at all.

a naive question, but are you sure that it just doesn't seem that way after Ina's latest commit, which commented out all print/debug statements?

@talavis
Copy link
Contributor

talavis commented Mar 30, 2022

I can't get it working in Gunicorn (which is used for production). I have a task that triggers correctly when running via flask (standard docker-compose), but it does not trigger when I run in Gunicorn. If we can't get it working in Gunicorn, I do not see a reason to use it at all.

a naive question, but are you sure that it just doesn't seem that way after Ina's latest commit, which commented out all print/debug statements?

@scheduler.task("interval", id="print_test", seconds=2, misfire_grace_time=1)
def print_test():
    with scheduler.app.app_context():
        from datetime import datetime
        with structlog.threadlocal.bound_threadlocal(
            who={"user": "print_test", "role": str(datetime.now())},
            by_whom={"user": "print_test", "role": "scheduler"},
        ):
            action_logger.info("apscheduler print_test")

Gives me the following when I start with flask:

{"who": {"user": "print_test", "role": "2022-03-30 09:29:20.873854"}, "by_whom": {"user": "print_test", "role": "scheduler"}, "event": "apscheduler print_test", "logger": "actions", "level": "info", "timestamp": "2022-03-30T09:29:20.873915Z"}

While I only get an empty file if I start with Gunicorn.

I tested using the action log after failing to get any output in gunicorn with normal flask.current_app.logger.error() calls.

Copy link
Contributor

@talavis talavis left a comment

Choose a reason for hiding this comment

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

With our current Gunicorn setup, there will be two schedulers running in parallell, meaning that each task is run twice each timestep. Using my earlier example:

{"who": {"user": "print_test", "role": "2022-03-30 10:49:47.573795"}, "by_whom": {"user": "print_test", "role": "scheduler"}, "event": "apscheduler print_test", "logger": "actions", "level": "info", "timestamp": "2022-03-30T10:49:47.573850Z"}
{"who": {"user": "print_test", "role": "2022-03-30 10:49:47.575475"}, "by_whom": {"user": "print_test", "role": "scheduler"}, "event": "apscheduler print_test", "logger": "actions", "level": "info", "timestamp": "2022-03-30T10:49:47.575616Z"}
{"who": {"user": "print_test", "role": "2022-03-30 10:49:49.577357"}, "by_whom": {"user": "print_test", "role": "scheduler"}, "event": "apscheduler print_test", "logger": "actions", "level": "info", "timestamp": "2022-03-30T10:49:49.577386Z"}
{"who": {"user": "print_test", "role": "2022-03-30 10:49:49.580273"}, "by_whom": {"user": "print_test", "role": "scheduler"}, "event": "apscheduler print_test", "logger": "actions", "level": "info", "timestamp": "2022-03-30T10:49:49.580313Z"}

Copy link
Contributor

@talavis talavis left a comment

Choose a reason for hiding this comment

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

See my suggestion. We need to discuss what to do with the gunicorn setup later.

Copy link
Contributor

@talavis talavis left a comment

Choose a reason for hiding this comment

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

It seems that the changes after 1.12.1 fixes the bug we see in our tests. But that fix seems to instead cause the issues in Gunicorn. Yay.

requirements.txt Outdated Show resolved Hide resolved
Ina and others added 5 commits March 30, 2022 14:49
Co-authored-by: Linus Östberg <linus.ostberg@scilifelab.uu.se>
Co-authored-by: Linus Östberg <linus.ostberg@scilifelab.uu.se>
@i-oden i-oden force-pushed the flask-apscheduler-correct-black branch from cf339e4 to 8a23aca Compare March 30, 2022 12:49
@i-oden i-oden self-assigned this Mar 30, 2022
@i-oden i-oden added the must have Cannot deliver on target date without this label Mar 30, 2022
@i-oden i-oden merged commit e25b8ee into dev Mar 30, 2022
@i-oden i-oden deleted the flask-apscheduler-correct-black branch March 30, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
must have Cannot deliver on target date without this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants