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

Persist dependencies so that Workflow#configure is not required on load #118

Merged

Conversation

noahfpf
Copy link
Contributor

@noahfpf noahfpf commented Jul 23, 2024

Previously, Workflow#configure was called every time a Workflow was instantiated. This could create broken dependency graphs, e.g. if a configure method sets up job dependencies based on some mutable data like a timestamp argument or a database value.

Instead, serialize workflow dependencies along with the rest of a workflow's data and reload it via Client#workflow_from_hash and tell Workflow#initialize not to run setup/configure.

Note that for backwards compatibility with workflows persisted before this change, the setup method will still be called if dependencies in the deserialized hash are nil.

@pokonski
Copy link
Contributor

Looks good! The only nitpick I have is about the name internals_, I'm thinking maybe internal_state or just state. What do you think?

@noahfpf
Copy link
Contributor Author

noahfpf commented Jul 24, 2024

Looks good! The only nitpick I have is about the name internals_, I'm thinking maybe internal_state or just state. What do you think?

Thanks!

Since kwargs to this method are also used by the library user to pass in workflow data it seems important to avoid any param name that they could reasonably choose. internal_state is probably unlikely enough, so I'll go with that.

…load

Previously, `Workflow#configure` was called every time a Workflow was
instantiated. This could create broken dependency graphs, e.g. if a
configure method sets up job dependencies based on some mutable data like
a timestamp argument or a database value.

Instead, serialize workflow dependencies along with the rest of a workflow's
data and reload it via `Client#workflow_from_hash` and tell `Workflow#initialize`
not to run setup/configure.

Note that for backwards compatibility with workflows persisted before this
change, the setup method will still be called if dependencies in the
deserialized hash are nil.
@noahfpf noahfpf force-pushed the persist-workflow-dependencies branch from 49df552 to c96287f Compare July 24, 2024 13:33
@pokonski pokonski merged commit bf3cc61 into chaps-io:master Jul 27, 2024
12 checks passed
@pokonski
Copy link
Contributor

Merged! Thank you ❤️ I'm thinking with all the recent additions we could make a 3.0 version

@noahfpf noahfpf deleted the persist-workflow-dependencies branch July 29, 2024 15:39
@noahfpf
Copy link
Contributor Author

noahfpf commented Aug 6, 2024

Merged! Thank you ❤️ I'm thinking with all the recent additions we could make a 3.0 version

Thanks for merging this in! I agree a 3.0 release makes sense, though hoping to get your eyes on a few other PRs first. Let me know if you'd like a hand writing up the changelog when the time comes.

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.

2 participants