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

feat: add ray ids to workflows, clean up types #787

Merged

Conversation

MasterPtato
Copy link
Contributor

Changes

Copy link
Contributor

graphite-app bot commented May 9, 2024

Your org requires the Graphite merge queue for merging into main

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

You can enable merging using labels in your Graphite merge queue settings.

@MasterPtato MasterPtato marked this pull request as ready for review May 9, 2024 21:26
Copy link
Member

@AngelOnFira AngelOnFira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial thoughts. These are more "stream of consiousness" than things that nessessarily need to be changed.

# Glossary

## Worker

A process that's running workflows.
A process that queries for pending workflows with a specific filter. Filter is based on which workflows are registered in the given worker's registry.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to imply that workflows don't need to be run on the same machine. Are
there any limitations to how this might scale?

Also, it would be cool to see a bit of a written example. Something akin to:

For example, there might be 3 workers, each with their own registry list:

Worker 1:
    - cpu-intensive
    - private-database
Worker 2:
    - private-database
    - public-database
Worker 3:
    - public-database

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what those lists are referring to in terms of an example


There are usually multiple workers running at the same time.
A collection of registered workflows.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it sound like I'd define a registry with several workflows
somewhere, then be able to re-use it in code to put several complex steps into a
single call.

It also sounds like it would be helpful to have a way to easily navigate all
existing registries in a similar way that I might go look for existing ops that do the
thing I want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then be able to re-use it in code to put several complex steps into a
single call

What do you mean by this?

Registries exist within code and do not need to be searched for anything by the developer. Its a way to tell the worker what workflows you want it to run. You define a registry in the /worker of each pkg and then in the monolith worker you import and merge them together when starting the workflow worker. They dont deal with ops at all


An instance of a node running a workflow. If re-running a workflow, it will be replaying events.
A block of code. Can fail or not fail, used simply for tidiness.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what "tidiness" means in this context. I'm also
curious about the Activity vs Operation distinction. What can an Activity do
that an operation can't?

Does this basically mean that it's more like separating functions? So an example
of an Activity might be "create a new user", and this uses the operations "find
existing user" and "create new user" and "send user email"?

@MasterPtato MasterPtato force-pushed the 05-09-fix_cast_workflow_errors_to_raw_global_errors branch from f9f206e to b6b8a23 Compare May 9, 2024 23:33
@MasterPtato MasterPtato force-pushed the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch from 4f009a2 to 02510dd Compare May 9, 2024 23:33
@MasterPtato MasterPtato force-pushed the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch from 02510dd to 23109a9 Compare May 9, 2024 23:43
Copy link
Member

@NathanFlurry NathanFlurry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

biggest thing is: use Box<serde_json::rawvalue> everywhere (essentially a string) instead of serde_json::Value (which does a bunch of heap allocs)

docs/libraries/workflow/OVERVIEW.md Outdated Show resolved Hide resolved
docs/libraries/workflow/OVERVIEW.md Outdated Show resolved Hide resolved
docs/libraries/workflow/OVERVIEW.md Outdated Show resolved Hide resolved
docs/libraries/workflow/OVERVIEW.md Show resolved Hide resolved
lib/chirp-workflow/core/src/db/mod.rs Show resolved Hide resolved
lib/chirp-workflow/core/src/db/mod.rs Show resolved Hide resolved
lib/chirp-workflow/core/src/db/mod.rs Show resolved Hide resolved
lib/chirp-workflow/core/src/db/mod.rs Show resolved Hide resolved
lib/chirp-workflow/core/src/db/mod.rs Show resolved Hide resolved
lib/chirp-workflow/core/src/signal.rs Outdated Show resolved Hide resolved
@MasterPtato MasterPtato force-pushed the 05-09-fix_cast_workflow_errors_to_raw_global_errors branch from b6b8a23 to 5edfdd0 Compare May 16, 2024 23:37
@MasterPtato MasterPtato force-pushed the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch from 23109a9 to a9ae382 Compare May 16, 2024 23:38
@MasterPtato MasterPtato force-pushed the 05-09-fix_cast_workflow_errors_to_raw_global_errors branch from 5edfdd0 to 4dd8e83 Compare May 17, 2024 00:10
@MasterPtato MasterPtato force-pushed the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch from a9ae382 to 06ae309 Compare May 17, 2024 00:10
Copy link
Contributor Author

See my comment on raw value

@MasterPtato MasterPtato force-pushed the 05-09-fix_cast_workflow_errors_to_raw_global_errors branch from 4dd8e83 to 393f088 Compare May 17, 2024 00:39
@MasterPtato MasterPtato force-pushed the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch from 06ae309 to 6c42c6d Compare May 17, 2024 00:39
@MasterPtato MasterPtato force-pushed the 05-09-fix_cast_workflow_errors_to_raw_global_errors branch from 393f088 to 1431bf5 Compare May 17, 2024 20:25
@MasterPtato MasterPtato force-pushed the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch from 6c42c6d to 33e1890 Compare May 17, 2024 20:25
@MasterPtato MasterPtato force-pushed the 05-09-fix_cast_workflow_errors_to_raw_global_errors branch from 1431bf5 to 8746cdf Compare May 30, 2024 16:44
@MasterPtato MasterPtato force-pushed the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch from 33e1890 to 76b70b5 Compare May 30, 2024 16:44
@MasterPtato MasterPtato force-pushed the 05-09-fix_cast_workflow_errors_to_raw_global_errors branch from 8746cdf to 3c9ae6f Compare June 4, 2024 18:53
@MasterPtato MasterPtato force-pushed the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch from 76b70b5 to f5e2153 Compare June 4, 2024 18:53
@MasterPtato MasterPtato force-pushed the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch from f5e2153 to 2833a80 Compare June 4, 2024 20:46
@MasterPtato MasterPtato force-pushed the 05-09-fix_cast_workflow_errors_to_raw_global_errors branch from 3c9ae6f to 56f9056 Compare June 4, 2024 21:59
@MasterPtato MasterPtato force-pushed the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch from 2833a80 to a923a66 Compare June 4, 2024 21:59
Copy link
Contributor

graphite-app bot commented Jun 5, 2024

Merge activity

<!-- Please make sure there is an issue that this PR is correlated to. -->

## Changes

<!-- If there are frontend changes, please include screenshots. -->
@MasterPtato MasterPtato force-pushed the 05-09-fix_cast_workflow_errors_to_raw_global_errors branch from 56f9056 to c90d939 Compare June 5, 2024 20:16
@MasterPtato MasterPtato force-pushed the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch from a923a66 to 3072bdc Compare June 5, 2024 20:17
@MasterPtato MasterPtato changed the base branch from 05-09-fix_cast_workflow_errors_to_raw_global_errors to main June 5, 2024 20:24
@graphite-app graphite-app bot merged commit 3072bdc into main Jun 5, 2024
6 of 10 checks passed
@graphite-app graphite-app bot deleted the 05-09-feat_add_ray_ids_to_workflows_clean_up_types branch June 5, 2024 20:26
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

3 participants