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

Feature: Dynamic Workflows #27

Merged
merged 114 commits into from
Jan 9, 2025
Merged

Feature: Dynamic Workflows #27

merged 114 commits into from
Jan 9, 2025

Conversation

koonpeng
Copy link
Collaborator

@koonpeng koonpeng commented Oct 2, 2024

New feature implementation

Implemented feature

Add support for dynamic workflows, dynamic workflows are workflows that is built at runtime.

Next steps

Add deserialization of dynamic workflows.

Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng koonpeng requested a review from mxgrey October 2, 2024 05:50
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng koonpeng changed the title Add service registry Feature: Dynamic Workflows Nov 6, 2024
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
src/diagram/join.rs Outdated Show resolved Hide resolved
@koonpeng koonpeng requested a review from mxgrey January 8, 2025 09:05
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

This review is primarily an audit of the use of .unwrap() inside the critical path.

It's imperative that we bring the risk of crashing/aborting/panicking as close to zero as possible, or else we undermine the crash-free exception-free selling points of Rust.

There will certainly be cases where we can guarantee that .unwrap() is perfectly safe, and in those places we should add clear documentation to explain how we know this.

But in general we should prefer to return Err results if we aren't positively 100% certain that the unwrap will not panic.

src/diagram/join.rs Outdated Show resolved Hide resolved
src/diagram.rs Show resolved Hide resolved
src/diagram/split_serialized.rs Show resolved Hide resolved
src/diagram/split_serialized.rs Show resolved Hide resolved
src/diagram/workflow_builder.rs Outdated Show resolved Hide resolved
src/diagram/workflow_builder.rs Outdated Show resolved Hide resolved
src/diagram/workflow_builder.rs Outdated Show resolved Hide resolved
src/diagram/workflow_builder.rs Outdated Show resolved Hide resolved
src/diagram/workflow_builder.rs Outdated Show resolved Hide resolved
src/diagram/workflow_builder.rs Outdated Show resolved Hide resolved
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng
Copy link
Collaborator Author

koonpeng commented Jan 9, 2025

It is hard to explain how the unwraps in workflow_builder.rs are safe. We usually access edges with a key that comes from vertices and both are built using the same data, so in theory it would never panic. But we do modify edges while building the workflow so the preconditions became very complex.

I added a UnknownDiagramError, for these unwraps, it captures the backtrace to help debug if it ever happens.

src/diagram.rs Outdated Show resolved Hide resolved
This reverts commit 8ef5fb5.

Back to capturing source file and line, backtrace are lost in some macros for some reason.

Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng koonpeng requested a review from mxgrey January 9, 2025 05:49
mxgrey and others added 4 commits January 9, 2025 14:46
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Thanks for all the rigorous iterating @koonpeng

I'm super happy with how this capability turned out. The JSON syntax is very clean and intuitive without limiting what can be done with it. The fact that the schema can be generated from native Rust structs is even better.

We still have plenty more operations to add, but this initial set is already very powerful, and most importantly it sets a strong foundation for us to continue building off of.

@mxgrey mxgrey merged commit 013dc62 into main Jan 9, 2025
6 checks passed
@mxgrey mxgrey deleted the koonpeng/service-registry branch January 9, 2025 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants