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.

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

add store_eager_result setting so eager tasks can store result on the backend #6614

Merged
merged 6 commits into from
Feb 24, 2021
Merged

add store_eager_result setting so eager tasks can store result on the backend #6614

merged 6 commits into from
Feb 24, 2021

Conversation

tomwojcik
Copy link
Contributor

Currently, if you use eager tasks and set that you do not want to ignore the result, it won't be saved on the backend.
There's literally no possible way to do that.

This PR adds a setting that will allow doing just that.

Imo that's a bug. Fixing it by allowing the task result to be saved when ignore result is false might break a lot of already existing projects so adding a new setting is the safest option.

Fixes #6476

@auvipy auvipy added this to the 5.1.0 milestone Jan 25, 2021
auvipy
auvipy reviewed Jan 25, 2021
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

looks good to me. did you check this manually that it's working?

@tomwojcik
Copy link
Contributor Author

looks good to me. did you check this manually that it's working?

Not yet, I plan to test it today against my project. Will get back to you with the confirmation.

@tomwojcik
Copy link
Contributor Author

@auvipy I confirm it is working! :)

@tomwojcik
Copy link
Contributor Author

tomwojcik commented Jan 25, 2021 •
edited

Though there is one issue, eager tasks do not store FAILURE results. Is that a blocker?

Added.

celery/celery/app/trace.py

Line 162 in 29eda05

store_errors = not eager

@lgtm-com
Copy link

lgtm-com bot commented Jan 25, 2021

This pull request fixes 1 alert when merging 9f91917 into 29eda05 - view on LGTM.com

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@auvipy
Copy link
Member

auvipy commented Jan 25, 2021

Though there is one issue, eager tasks do not store FAILURE results. Is that a blocker?

Added.

celery/celery/app/trace.py

Line 162 in 29eda05

store_errors = not eager

thanks for checking. since it's a old issue so we had confusion in mind, so manual testing was needed

@tomwojcik
Copy link
Contributor Author

thanks for checking. since it's a old issue so we had confusion in mind, so manual testing was needed

Sure. Just to make it clear, I am currently using this version in my project and I find it to be working flawlessly. I'm not sure what old issue you are talking about.

Please let me know if you need something else from me.

@auvipy
Copy link
Member

auvipy commented Jan 26, 2021

thanks for checking. since it's a old issue so we had confusion in mind, so manual testing was needed

Sure. Just to make it clear, I am currently using this version in my project and I find it to be working flawlessly. I'm not sure what old issue you are talking about.

Please let me know if you need something else from me.

I meant this is a very old issue so was bit cautious. I think we are good. waiting for some response from other maintainers

@lgtm-com
Copy link

lgtm-com bot commented Jan 27, 2021

This pull request introduces 1 alert and fixes 1 when merging 9bf78ec into 29eda05 - view on LGTM.com

new alerts:

  • 1 for Non-exception in 'except' clause

fixed alerts:

  • 1 for Module is imported with 'import' and 'import from'

@matusvalo
Copy link
Member

As long as I understand the main motivation of this PR is to support testing. Maybe we can update also this documentation:

https://docs.celeryproject.org/en/latest/userguide/testing.html

@tomwojcik
Copy link
Contributor Author

As long as I understand the main motivation of this PR is to support testing. Maybe we can update also this documentation:

https://docs.celeryproject.org/en/latest/userguide/testing.html

Yes, makes sense. Will do.

@tomwojcik
6476 docs(results): mention task_store_eager_result in docs 'testin…
6ca4f3a 
…g' section where it's mention why/how use eager tasks when testing
@tomwojcik
Copy link
Contributor Author

Sorry it took so long. For the past 3 weeks I use it in my project, everything is fine. I believe tests exhaust all new cases and there will be no regression as it is a new setting. I think there's nothing more to be added in the docs either.

Please let me know if I can do anything else.

@lgtm-com
Copy link

lgtm-com bot commented Feb 20, 2021

This pull request introduces 1 alert and fixes 1 when merging 6ca4f3a into 1eade4c - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for Non-exception in 'except' clause

matusvalo
matusvalo previously approved these changes Feb 20, 2021
View reviewed changes
Copy link
Member

@matusvalo matusvalo left a comment

Choose a reason for hiding this comment

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

LGTM for me.

auvipy
auvipy reviewed Feb 21, 2021
View reviewed changes
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

IMHO we should now focus on 5.1 new features to master

docs/userguide/configuration.rst Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Feb 21, 2021

This pull request introduces 1 alert and fixes 1 when merging 6927a04 into 1eade4c - view on LGTM.com

new alerts:

  • 1 for Unused import

fixed alerts:

  • 1 for Non-exception in 'except' clause

@auvipy auvipy merged commit e62dc67 into celery:master Feb 24, 2021
@auvipy
Copy link
Member

auvipy commented Feb 24, 2021

merging this as 5.0 release almost 6 months back. we should combine 5.0.6 & 5.1.0 release to 5.1.0 this time. hope everyone is ok with that reasoning. I will try to help to write the release notes so that releasing would be easier for whoever wants to release them.

@thedrow thedrow added this to Done in Celery 5.1.0 Feb 24, 2021
thedrow
thedrow reviewed Feb 28, 2021
View reviewed changes
Copy link
Member

@thedrow thedrow left a comment

Choose a reason for hiding this comment

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

Thank you. Sorry I didn't get to reviewing this before it got merge.
Looks great!

jeyrce pushed a commit to jeyrce/celery that referenced this pull request Aug 25, 2021
@tomwojcik @jeyrce
add store_eager_result setting so eager tasks can store result on the…
ddd1922 
… backend (celery#6614)

* 6476 feat(setting): add setting `store_eager_result` that allows to store eagerly executed tasks results on the backend

* 6476 style(setting): fix flake8 and sphinx lint

* 6476 feat(results): add support for saving failed task results on the backend

* 6476 docs(results): reword new setting definition in docs, add myself to contributors

* 6476 docs(results): mention `task_store_eager_result` in docs 'testing' section where it's mention why/how use eager tasks when testing

* 6476 docs(results): add versionadded 5.1 in docs under task_store_eager_result
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thedrow thedrow thedrow left review comments

@auvipy auvipy auvipy approved these changes

@matusvalo matusvalo Awaiting requested review from matusvalo

Assignees
No one assigned
Labels
Category: Results Backend Component: Configuration PR Type: Enhancement
Projects
No open projects
Celery 5.1.0
  
Done
Milestone
5.1.0
Development

Successfully merging this pull request may close these issues.

publish_result is False when tasks are eager and and we DON'T want to ignore results
4 participants
@tomwojcik @auvipy @matusvalo @thedrow