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

Pipelines with Cycles: e2e testing, and pipeline validation #920

Merged
merged 31 commits into from
Aug 8, 2023

Conversation

juliev0
Copy link
Contributor

@juliev0 juliev0 commented Aug 5, 2023

Fixes: #934

We will allow Cycles in Pipelines as long as there's no Reduce Vertex at the point of the Cycle or to the right of it (to avoid issues of late data after close-of-book).

This PR includes:

  • e2e tests of valid Cycles
  • Validation to prevent invalid Cycles

As a side note, I have observed how the cycle looks in the UI, and I believe this does need modification (i.e. if Vertex A sends to Vertex B, but Vertex B cycles back to A, there should probably be a second line (edge); and if Vertex B cycles to itself, that probably needs a line (edge))

Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
@juliev0
Copy link
Contributor Author

juliev0 commented Aug 5, 2023

I think this is failing because maybe the image for this hasn't been published to quay.io?

Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
@juliev0 juliev0 marked this pull request as ready for review August 7, 2023 22:39
@vigith
Copy link
Member

vigith commented Aug 8, 2023

Can you add one spec to examples/ folder which will include both cycle-to-self and cycle-to-prev ?

Signed-off-by: Julie Vogelman <[email protected]>
@juliev0
Copy link
Contributor Author

juliev0 commented Aug 8, 2023

Can you add one spec to examples/ folder which will include both cycle-to-self and cycle-to-prev ?

I added one for each - is that okay?

Signed-off-by: Julie Vogelman <[email protected]>
// getCycles locates the vertices where there's a Cycle, if any
// eg. if A->B->A, then return A
// Since there are multiple Sources, and since each Source produces a Tree, then we can return multiple Cycles
func getCycles(pipelineSpec *dfv1.PipelineSpec) (map[string]struct{}, error) {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: what will be the error for this func? is it only "no vertex found", isn't it already covered in 320?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Well, I suppose there's nothing to prevent getCycles() from being called from somewhere else, so for completeness I think I'll leave it in.

Signed-off-by: Julie Vogelman <[email protected]>
@juliev0 juliev0 merged commit 2d6112b into numaproj:main Aug 8, 2023
14 checks passed
@juliev0 juliev0 deleted the cycle branch August 8, 2023 19:33
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.

Cycles for Join Vertex
2 participants