Skip to content
This repository was archived by the owner on Feb 20, 2025. It is now read-only.

Conversation

@mrkaye97
Copy link

@mrkaye97 mrkaye97 commented Jan 30, 2025

This is a lot, but please read it - I need some feedback in a couple of spots :)

This PR implements the V2 SDK's definitions of workflows and steps (standalone functions soon to come). There's a lot going on here, sorry about that 😄

First, some things less related to workflows:

  1. There's a new Step class that's has a generic param (the return type), which implements call and acall methods and has all the same parameters as the old steps, just as a class as opposed to as a function. This helps type checking a lot - we can know the return type of the step (at least in generic terms) in the other places in the code, can access attributes correctly, and so on.
  2. The Step is passed around everywhere now, so there's much less of a notion of an "action function". Instead, we just pass the step and figure out how to call it depending on if it's is_async_function is True or False. This is also a dramatic improvement for type checking, e.g. here or here
  3. When we go to register a workflow, all we need to do is loop over each of its steps (can look for anything with isinstance(..., Step))

For the workflows themselves:

  1. There's a new way of working with Hatchet workflows: We now have a WorkflowConfig, which is configuration for a workflow (name, etc.), we have a WorkflowDeclaration, which is generic with the type of the input to the workflow, and has methods for run, etc. (just run and workflow_input right now but will extend), and we have BaseWorkflowImpl, which is a base class intended to be inherited from in a Pydantic-y fashion, and have config passed as a class variable (similar to how Pydantic handles model_config by allowing you to pass a ConfigDict).
  2. The Hatchet class now has an instance method declare_workflow, which takes workflow config keyword arguments and returns a typed DeclarativeWorkflow object, which you can pass to your workflow class that inherits from BaseWorkflowImpl. Importantly, this behavior is opt-in. If you don't pass any config, we fall back to the old workflow behavior where you used @workflow() with no params

Check out the simple and v2 examples for more of a sense of how this is intended to work. In the v2 example, the type checker is happy all the way through - it knows the type of the workflow input when it reads it from the context in the step, it knows the type of the input when we go to trigger the workflow, it infers the name of the workflow to trigger, and so on.

Important feedback I'd love:

  1. Naming is hard - what do you think of the names of these concepts - especially the Declaration / Impl / abuse of Workflow?
  2. Does the usage pattern here make sense? From my PoV, this isn't much overhead and gives the user a lot of power / control over their ability to use our SDK in a more type-safe manner throughout the codebase

Miscellaneous notes:

  1. There's a lot of hacking around circular imports here by using if TYPE_CHECKING and such - don't worry about that for now. I'll do my best to fix it later.
  2. There are definitely a bunch of bugs here (e.g. I stumbled upon the namespace field needing to be deleted before triggering a workflow when I went to do that - I'm sure we'll find more as I convert tests over and such).

@mrkaye97 mrkaye97 marked this pull request as ready for review January 30, 2025 21:45
@mrkaye97 mrkaye97 requested review from abelanger5 and grutt January 30, 2025 21:45
@grutt
Copy link

grutt commented Feb 3, 2025

Nice! I think this makes a lot of sense and I am very happy with the cleanup for registration and types!

I want to discuss briefly implications of #7 as I think we might want to consider moving in the direction of "all things are functions" where dags are functions that orchestrate functions... lets discuss there for a bit.

Wrt naming convention, I'm not sure what is most pythonic but i don't like Impl and perhaps we should have Hatchet in more base things?

@mrkaye97
Copy link
Author

mrkaye97 commented Feb 3, 2025

Nice! I think this makes a lot of sense and I am very happy with the cleanup for registration and types!

I want to discuss briefly implications of #7 as I think we might want to consider moving in the direction of "all things are functions" where dags are functions that orchestrate functions... lets discuss there for a bit.

Wrt naming convention, I'm not sure what is most pythonic but i don't like Impl and perhaps we should have Hatchet in more base things?

Great, thanks for the feedback!

Re: naming - I don't think adding Hatchet adds that much value since you'd be importing it from hatchet_sdk (obviously), but I agree about Impl. What do you think of just BaseWorkflow, to be in line with Pydantic's BaseModel? I think that'd feel familiar to a lot of people, and would get the point across?

@grutt
Copy link

grutt commented Feb 3, 2025

BaseWorkflow makes sense to me.

@mrkaye97 mrkaye97 merged commit d079700 into main Feb 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants