BUG Error while deleting a task

[carlosporta]

I am trying to delete a task, but Rocketry returns a KeyError.

import asyncio
from rocketry import Rocketry
from rocketry.conds import secondly

app = Rocketry()

@app.task(secondly, name='foo')
async def foo():
    ...


async def main():
    asyncio.create_task(app.serve())
    app.session.remove_task('foo')
    # app.session.tasks.remove('foo')


if __name__ == '__main__':
    asyncio.run(main())

[Miksus]

Thanks for reporting this!

The method does not make much sense:

class Session(RedBase):
...
    def remove_task(self, task: Union['Task', str]):
        if isinstance(task, str):
            task = self[task]
        self.session.tasks.remove(task)

We are already in the session, there is no need to go to another session. The back story of this is that I used to think the "session" object is something that's an attribute everywhere. I later thought to make the internals pass this when needed to reduce coupling. It's most likely a typo that the session is there. This feature is tested but the test passed most likely because it uses the "default" session (the session default is deprecated).

The nice thing is that the tasks are stored in a simple set, the tasks are hashable (the name should be unique anyways). Quick fix is simply remove the task manually:

app.session.tasks.remove(app.session['foo'])

Thanks again for reporting. The fix is simple (just take the extra session off from the method) but would be nice to do a test to the app level in which this bug would have been caught. A good easy issue if someone is interested but I can also fix it in the upcoming days.

A bit embarrassed about this bug.

[rohansh-tty]

Hey @Miksus, can I take up this issue?

Based on your approach, I have changed the session module's remove_task() method.

def remove_task(self, task: Union['Task', str]):
        if isinstance(task, str):
            task = self[task]
        self.tasks.remove(task)

Is this correct? I also tried running test cases (in test/ folder) on this method and it works fine. Here's the result

[Miksus]

@rohansh-tty, ye, go ahead! I think there was a PR but it had a bit more (tests failed due to that) I suppose it got inactive. What you have there is correct I think.

I think if you do the above and add a test for this it's fine for me to approve the PR. But you can do the PR with the above changes and I can advise/sketch a test to add to with the PR. I think this bug probably would have been exposed in the tests if they tested the removing in the app level (like these: https://github.com/Miksus/rocketry/tree/master/rocketry/test/app).

But I can help with coming up with the proper unit test if you open the PR 👍