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

Support Ignored State #64

Merged
merged 10 commits into from
Oct 28, 2020
Merged

Support Ignored State #64

merged 10 commits into from
Oct 28, 2020

Conversation

EJH2
Copy link
Collaborator

@EJH2 EJH2 commented Oct 28, 2020

Part 3 of 4 in the saga that is #54. Significant changes in this PR:

  • Added IGNORED state support to Progress.get_info().
  • Added onIgnored funtion to JS (as well as the associated color).
    • Default configuration looks like so:
      image
      If a user specifies an error message (i.e. doing raise Ignore('lol')), it will use the error message instead like so:
      image
  • Added a cleaner and clearer default settings visualization for barColors, taking the form of a table.
  • Added a post-run handler to the base package to make sure that the HTTP progress bars properly receive the ignored event.
    • This is an unfortunate but necessary change, as without the post-run handler updating the backend, the backend will only show the last progress update it received. This is obviously less than ideal since the intent is to disconnect the task.

All that's left from the aforementioned issue is to investigate the behavior of store_errors_even_if_ignored, although I believe Omar was going to take a crack at that. Suggestions welcome.

@EJH2 EJH2 requested review from czue and OmarWKH October 28, 2020 05:40
Copy link
Owner

@czue czue left a comment

Choose a reason for hiding this comment

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

This looks great - appreciate you breaking everything up by commit and also the README improvements! 👏 👏

Copy link
Collaborator

@OmarWKH OmarWKH left a comment

Choose a reason for hiding this comment

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

This is great.

My preference is against a solution that requires updating the state after being ignored. But I understand that it might not be doable otherwise :)

def task_postrun_handler(**kwargs):
"""Runs after a task has finished. This will update the result backend to include the IGNORED result state.

Necessary for HTTP to properly receive ignored task event."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does WS get an IGNORED state without this post handler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the WS receives the IGNORED state from it's own post handler, this is necessary purely to update the state on the backend so that the AsyncResult in the HTTP view can be notified that it's been ignored, as the point of the IGNORED state is to not save anything to the backend.

@OmarWKH
Copy link
Collaborator

OmarWKH commented Oct 28, 2020

All that's left from the aforementioned issue is to investigate the behavior of store_errors_even_if_ignored, although I believe Omar was going to take a crack at that. Suggestions welcome.

Sorry. Might be a while before I test it. But however it works, it shouldn't block this PR.

@EJH2 EJH2 merged commit 018f059 into master Oct 28, 2020
@EJH2 EJH2 deleted the feat-support-ignored branch October 28, 2020 22:41
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.

3 participants