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

update examples #6

Closed
zzstoatzz opened this issue Feb 5, 2025 · 0 comments · Fixed by #7
Closed

update examples #6

zzstoatzz opened this issue Feb 5, 2025 · 0 comments · Fixed by #7
Assignees

Comments

@zzstoatzz
Copy link
Contributor

zzstoatzz commented Feb 5, 2025

continuing the conversation started in #1

cc @EmilRex

gripe

I don't think we ought to recommend the patterns shown in return_custom_state.py and update_flow_run_state_from_hook.py. I imagine these were informed by real use cases, but I would say both of them are anti-patterns. The former should simply use the retry kwargs (retry_delay_seconds and retry_condition_fn) and the latter seems to be a crutch for some infra issue that violates the idea of a state change hook (changing the state itself as a side effect of entering a state is kinda goofy). happy to discuss further

justification

They are indeed informed by real use cases. They certainly may need to be tweaked, but I think they both demonstrate important concepts:
return_custom_state.py demonstrates what returning a state does to a flow as opposed to returning an arbitrary result. FWIW I thought it was an elegant way to do a custom retry. What makes it an anti-pattern?
update_flow_run_state_from_hook.py demonstrates what setting a custom state via the API client looks like. Is there another state we could set that wouldn't be an anti-pattern?


What makes it an anti-pattern?

I'd say this is exploiting implementation details instead of using first-class features like retry_delay_seconds and retry_condition_fn.

Is there another state we could set that wouldn't be an anti-pattern?

I think any state mutation of task/flow run via API in it's own state hook is an anti-pattern and indicative of something we need to change about the SDK. if there is a case where you want to post-hoc update the state of a run (I would suggest this is not a defensible goal in most cases) then I would say you should use the client on the resulting state, instead of doing it from the state hook which itself is supposed to respond to state changes

@zzstoatzz zzstoatzz self-assigned this Feb 5, 2025
EmilRex added a commit that referenced this issue Feb 11, 2025
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 a pull request may close this issue.

1 participant