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

Add test to ensure flows can be deserialized prior to depoy #1397

Merged
merged 3 commits into from
Aug 23, 2019

Conversation

jlowin
Copy link
Member

@jlowin jlowin commented Aug 23, 2019

Thanks for contributing to Prefect!

Please describe your work and make sure your PR:

  • adds new tests (if appropriate)
  • updates CHANGELOG.md (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

Note that your PR will not be reviewed unless all three boxes are checked.

What does this PR change?

A user had a situation in which he modified attributes of a task in a way that Core would have prevented if the same arguments were provided at task initialization. Specifically: setting a max_retries value without a corresponding retry_delay. The user then deployed the flow to Cloud. All healthchecks passed, including serialization. However, Cloud (correctly, in this case) rejected the flow as invalid.

The issue is that the serialization healthcheck is a cloudpickle serialization check, not a JSON serialization check. In this case, the Flow was indeed a valid Python object -- there was nothing Pythonically "wrong" with assigning max_retries a value (though it would have caused an issue at runtime!). Cloudpickle serialization doesn't call __init__(), so it didn't apply the validation logic. JSON deserialization DOES call init(), so it validates that arguments (like max_retries and retry_delay) are appropriately set.

In order to avoid this poor user experience, this PR adds a check to make sure that the serialized flow can also be immediately deserialized.

Why is this PR important?

User experience, finding out right away if a flow's serialized form is invalid!

@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #1397 into master will increase coverage by <.01%.
The diff coverage is 85.71%.

@jlowin jlowin merged commit ee949f9 into master Aug 23, 2019
@jlowin jlowin deleted the json-healthcheck branch August 23, 2019 17: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.

None yet

2 participants