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

[WIP] Remove over-engineered aspect of prefect context #703

Closed
wants to merge 4 commits into from

Conversation

cicdw
Copy link
Member

@cicdw cicdw commented Mar 6, 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)

What does this PR change?

This PR removes an over-engineered aspect of Prefect context - they are no longer thread local objects.

Why is this PR important?

Two things were happening to me:

  • I would run with prefect.context(secrets=...): flow.run() and dask would attempt to pickle the context object and fail
  • I wrote custom __setstate__ and __getstate__ methods, and despite this some of my updated keys in context were not being found within the task runners

Removing threading.local appeared to fix both of these problems in a simple way, and I don't think it's necessary for context to be threadsafe (please correct me if I'm wrong). Our use of context is always that downstream function calls retrieve information from context without updating context.

@cicdw cicdw changed the title Remove over-engineered aspect of prefect context [WIP] Remove over-engineered aspect of prefect context Mar 6, 2019
@cicdw
Copy link
Member Author

cicdw commented Mar 6, 2019

Closing; I now remember why we use threading.local and it's related to the fact that context can become polluted in tests due to multithreading and cause bizarre context-related test failures.

For posterity: the test failures are caused by raise_on_exception somehow persisting in context as True and other tests which simply expect Failed states actually reraise their exceptions.

Similarly, Parameter values can be off because parameters pull their values from context.

@cicdw cicdw closed this Mar 6, 2019
@cicdw cicdw deleted the context-changes branch March 25, 2019 22:16
zanieb added a commit that referenced this pull request Jan 12, 2022
Use new setup-python pip caching instead of custom
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.

1 participant