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

[BD-32] feat: add async feature to Hooks Extension Framework tooling #8

Closed
wants to merge 4 commits into from

Conversation

mariajgrimaldi
Copy link

@mariajgrimaldi mariajgrimaldi commented Mar 26, 2021

Description:

This PR allows the Hooks Extension Framework: Pipeline (check out its ADR) to run asynchronously using celery tasks.

JIRA:

XXX-XXXX

Dependencies:

This PR depends on Hooks Extension Framework: Tooling

Merge deadline:

None

Testing instructions:

TODO: make test repository usable for aysnc

Reviewers:

  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPi after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns:

  • Will there be a problem with adding celery? Adding it will cause other applications to crash?

@mariajgrimaldi mariajgrimaldi changed the base branch from master to MJG/hooks_pipeline_tool March 26, 2021 17:33
@mariajgrimaldi mariajgrimaldi force-pushed the MJG/add_async_feature branch from 6f4ac40 to 349ff19 Compare March 26, 2021 17:36
from .exceptions import HookException
from .utils import get_functions_for_pipeline

log = getLogger(__name__)


@shared_task()
Copy link
Author

@mariajgrimaldi mariajgrimaldi Mar 26, 2021

Choose a reason for hiding this comment

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

Something interesting I found in edx-celeryutils was a BaseTask called LoggedTask, wouldn't be useful to have that extra logging?

@@ -24,7 +24,7 @@ def setUp(self):
}
self.pipeline = Mock()

@patch("edx_django_utils.hooks.pipeline.get_functions_for_pipeline")
@patch("edx_django_utils.hooks.tasks.get_functions_for_pipeline")
Copy link
Author

@mariajgrimaldi mariajgrimaldi Mar 26, 2021

Choose a reason for hiding this comment

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

here just changing the pipeline location from pipeline.py to tasks.py

log.exception(
"An error ocurred in trigger_filter while executing `run pipeline` with arguments: %s, %s.",
str(args),
str(kwargs),
Copy link
Author

@mariajgrimaldi mariajgrimaldi Mar 26, 2021

Choose a reason for hiding this comment

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

maybe this won't be needed?

Copy link
Member

Choose a reason for hiding this comment

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

It makes no difference what the pipeline is at this point right?
I mean, we are sure the pipeline object is never the source of this error. It is always the serialization, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it should always be serialization. I'll add it to the message!

@mariajgrimaldi
Copy link
Author

The discuss and OEP feedback has led us to remove native support for async.

It is therefore preferable for events to follow the pattern:

Listen on a signal and then create the async task

But that will be the responsibility of whoever writes the action.

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.

2 participants