Skip to content

Workflow validation#1223

Merged
antho1404 merged 18 commits intodevfrom
feature/workflow-validation
Aug 15, 2019
Merged

Workflow validation#1223
antho1404 merged 18 commits intodevfrom
feature/workflow-validation

Conversation

@antho1404
Copy link
Copy Markdown
Member

Add workflow validation:

  • Basic validation of the required attributes
  • Graph structure validation:
    • Do not allow graph with cycles
    • Do not allow graph with multiple roots, (non-connected graphs)
    • Do not allow graph with nodes that contain more than one parent

With this validation, we are allowing only directed tree graphs which is a family of DAG.

You can use the test from this PR: #1209

@antho1404 antho1404 added the enhancement New feature or request label Aug 13, 2019
@antho1404 antho1404 added this to the next milestone Aug 13, 2019
@antho1404 antho1404 self-assigned this Aug 13, 2019
Copy link
Copy Markdown
Contributor

@krhubert krhubert left a comment

Choose a reason for hiding this comment

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

With this PR a lot of workflow methods handle the graph and not workflow itself. As in previous PR I would love to see the part where the graph is a separate struct.

Comment thread workflow/workflow.go Outdated
return fmt.Errorf("non connected graph")
}
if !w.isMonoParental() {
return fmt.Errorf("non mono-parental graph")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've never see a mono-parental word to describe a graph, can you point out the article/algorithm where they use such terminology?

Suggested change
return fmt.Errorf("non mono-parental graph")
return fmt.Errorf("the edge %s has to or more parents")

Also, a method name can be changed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is not the right term, I couldn't find a better name. If you know any name that describes that each node should have only one parent I'm really happy to change this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

changed the function to maximumParentNode(max int) that returns true when all the nodes have less than max parents.

Comment thread workflow/workflow.go Outdated
Comment thread workflow/algorithm.go Outdated
Comment thread workflow/algorithm.go Outdated
Comment thread workflow/algorithm.go Outdated
Comment thread workflow/algorithm.go
Comment thread workflow/algorithm.go Outdated
Comment thread workflow/algorithm.go Outdated
@antho1404
Copy link
Copy Markdown
Member Author

With this PR a lot of workflow methods handle the graph and not workflow itself. As in previous PR I would love to see the part where the graph is a separate struct.

I agree that's why I started to move all the graph-related stuff in the algorithm file. I don't want to charge too much this PR and also I have to have a look at https://github.com/gonum/gonum/tree/master/graph that might replace all. So this will certainly change soon.

@antho1404 antho1404 force-pushed the feature/workflow-validation branch from 1f81955 to a15be27 Compare August 14, 2019 09:13
Copy link
Copy Markdown
Member

@NicolasMahe NicolasMahe left a comment

Choose a reason for hiding this comment

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

it seems good except the function hasNodes that return the opposite of what the function's name is saying. The name should also by singular hasNode has it should true if at least 1 node exist.

Comment thread workflow/algorithm.go Outdated
Comment thread workflow/algorithm.go Outdated
Comment thread workflow/algorithm.go Outdated
Comment thread workflow/algorithm.go Outdated
Comment thread workflow/algorithm_test.go Outdated
Comment thread workflow/algorithm_test.go Outdated
Comment thread workflow/algorithm_test.go Outdated
@antho1404 antho1404 force-pushed the feature/workflow-validation branch from 14a43ff to cf7966d Compare August 15, 2019 04:19
antho1404 and others added 2 commits August 15, 2019 11:20
Comment thread workflow/algorithm.go Outdated
@antho1404 antho1404 merged commit 4bc708e into dev Aug 15, 2019
@antho1404 antho1404 deleted the feature/workflow-validation branch August 15, 2019 09:42
@NicolasMahe NicolasMahe modified the milestones: next, v0.13.0 Aug 16, 2019
@NicolasMahe NicolasMahe added the release:add Pull requests that add something label Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request release:add Pull requests that add something

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants