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

Pickling exceptions #8704

Merged
merged 11 commits into from
Jun 17, 2021
Merged

Pickling exceptions #8704

merged 11 commits into from
Jun 17, 2021

Conversation

Mazyod
Copy link
Contributor

@Mazyod Mazyod commented May 17, 2021

Proposed changes:

  • Fix error thrown when attempting to pickle UnsupportedModelError and InvalidModelError

I needed to pass exceptions from background processes to be handled in the main process, but it was these exceptions were causing issues during pickling. The fix is easy, and hopefully a better practice especially for a project like rasa which relies on multiprocessing.

Previous error:

def test_exception_pickling():
        exception = UnsupportedModelError("test run")
>       cycled_exception = pickle.loads(pickle.dumps(exception))
E       TypeError: __init__() missing 1 required positional argument: 'message'

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

Mazyod added 4 commits May 17, 2021 18:15
For UnsupportedModelError, and InvalidModelError
docstring copied from `AgentNotReady` class
@Mazyod
Copy link
Contributor Author

Mazyod commented May 18, 2021

Another option is to make message an optional argument:
https://stackoverflow.com/a/16245182/456434

@sara-tagger sara-tagger requested a review from tttthomasssss May 18, 2021 06:00
@sara-tagger
Copy link
Collaborator

Thanks for submitting a pull request 🚀 @tttthomasssss will take a look at it as soon as possible ✨

@Mazyod
Copy link
Contributor Author

Mazyod commented Jun 8, 2021

would appreciate any feedback on this PR

Copy link
Contributor

@tttthomasssss tttthomasssss left a comment

Choose a reason for hiding this comment

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

Hey, apologies this took so long. Thanks very much for submitting this PR.

All looks good codewise - great work 🎸 ! Could you just expand on the purpose of being able to pickle the exceptions in the changelog? I.e. just a line or two on why this is needed and what use-case it covers.

@Mazyod
Copy link
Contributor Author

Mazyod commented Jun 17, 2021

Appreciate it @tttthomasssss. Certainly, I have added the purpose for making exceptions pickleable. I hope the explanation would suffice, otherwise I could try adding more information.

Copy link
Contributor

@tttthomasssss tttthomasssss left a comment

Choose a reason for hiding this comment

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

Awesome, thanks very much for the lightning fast resolution 🚀

@tttthomasssss tttthomasssss enabled auto-merge June 17, 2021 11:19
@tttthomasssss tttthomasssss merged commit 1137e29 into RasaHQ:main Jun 17, 2021
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