dmontagu / fastapi-utils

Reusable utilities for FastAPI
MIT License
1.89k stars 165 forks source link

[BUG] @repeat_every() does not run without @app.on_event('startup') decorator #256

Closed caseyjohnsonwv closed 3 months ago

caseyjohnsonwv commented 2 years ago

Describe the bug The @repeat_every() decorator does not trigger the function it decorates unless the @app.on_event('startup') decorator is also present.

To Reproduce I created this sample main.py to show the issue I've been seeing.

  1. Copy the below and run python3 main.py. The app will create a local file test.txt and write a Unix timestamp to it every second.
  2. Stop the app and comment out @app.on_event('startup')
  3. Run the app again. The file test.txt is no longer being updated. If you delete this file and restart the app, it will never be created.
    
    import time
    from fastapi import FastAPI
    from fastapi_utils.tasks import repeat_every
    import uvicorn

app = FastAPI()

@app.on_event('startup') @repeat_every(seconds=1) def do_something(): with open('test.txt', 'w') as f: f.write(f"{time.time():.0f}")

if name == 'main': uvicorn.run('main:app')



**Expected behavior**
The function `do_something()` should be writing to the file every 1 second, even with `@app.on_event('startup')` commented out.

**Environment:**
 - OS: Windows 10
 - FastAPI Utils: 0.2.1
 -  FastAPI: 0.75.0
 - Pydantic: 1.9.0
- Python: 3.10.4
clemens-tolboom commented 2 years ago

I can confirm this. Even swapping the two

from (working)

@app.on_event('startup')
@repeat_every(seconds=1)

into (prints once)

@repeat_every(seconds=1)
@app.on_event('startup')

I changed the test into

from fastapi import FastAPI
from fastapi_utils.tasks import repeat_every
import uvicorn

app = FastAPI()

@app.on_event('startup')
@repeat_every(seconds=0.1, max_repetitions=80)
def do_something():
    print(".", end='')

if __name__ == '__main__':
    uvicorn.run(app)
vkomodey commented 2 years ago

the same here. works only with startup event

mvoitko commented 1 year ago

I we don't use @app.on_event('startup') how do the app know when to start the task?

lennpet commented 1 year ago

My suggestion would be that the framework scans for all decorator @repeat_every at startup and call them, preferable after that all @app.on_event("startup") has been executed. Comparing with Spring framework that has @scheduled that does exactly the same. Where you can decorate multiple functions/tasks that will be called on at startup. @scheduled decorator can also be configured with an initial delay, that is very nice to have.

znstahl commented 1 year ago

I we don't use @app.on_event('startup') how do the app know when to start the task?

The whole point of writing APIs in frameworks like FastAPI is for them to be opinionated. It's also pretty standard for scheduling frameworks to allow specifying a schedule without explicitly specifying a start time, which then by convention equals to "whenever you can start it ASAP".

It's also pretty pointless to require multiple decorators if one of them will be followed by the other in 99,9% (if not 100%) of the cases.

caseyjohnsonwv commented 1 year ago

I we don't use @app.on_event('startup') how do the app know when to start the task?

It's also pretty pointless to require multiple decorators if one of them will be followed by the other in 99,9% (if not 100%) of the cases.

I think a great example of this in practice is Apache Airflow, which requires a datetime object to specify the earliest invocation of a DAG. This datetime can be in the past, meaning "start this as soon as possible." It would make sense for the framework to do one or both of the following:

clemens-tolboom commented 1 year ago

'decorators are wrappers around' and in this case they come from two different tools in the order bottom to top.

Maybe fastapi-util could have 'auto decorated' it but that would mean you can never @repeat_every using a different decorator, ie 'after the 20th request'.

FWIW I learned the order (and more) from https://www.youtube.com/watch?v=QH5fw9kxDQA

AdityaSoni19031997 commented 6 months ago

This is not true, you need to trigger the repeatable event on a event loop and then it will start running on it's own. It's mentioned in the docs.

yuval9313 commented 3 months ago

Fixed in #291 and version 0.6.0

imancrsrk commented 3 months ago

Fixed in #291 and version 0.6.0

I'm using 0.6.0 and it's not fixed. I can confirm. It also doesn't seamlessly work with lifespan which is the recommended way to configure event handlers as on_event is deprecated.

@asynccontextmanager
async def lifespan(app: FastAPI):
     app.add_event_handler("startup", refresh_cache)
     yield

[UPDATE]

I finally got it working with lifespan as follows -

@repeat_every(seconds=10, logger=logger, raise_exceptions=True)  # 5 mins
async def foo():
    print("yay...")

app = FastAPI()
app.add_event_handler("startup", refresh_cache)
yuval9313 commented 3 months ago

Is that not the way you intended? What change you think need to happen?

drewmarshburn commented 2 weeks ago

I was not able to use add_event_handler to get this to work with the lifespan, but I was able to call the function from within the lifespan. After doing so, it recurs.

@repeat_every(seconds=1)
def example_repeat():
    print("example")
async def db_lifespan(app: FastAPI):
    # Db connection...

    # This must be executed on startup (the decorator relies on this being called once to start the loop)
    await example_repeat()

    yield

    # DB close...