Skip to content

Conversation

drom
Copy link

@drom drom commented Aug 5, 2015

No description provided.

Copy link
Owner

Choose a reason for hiding this comment

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

Not sure why jscs did not complain, but the indent is off here.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Will fix.

@indutny
Copy link
Owner

indutny commented Aug 5, 2015

Few nits, otherwise looking good. Thank you!

Copy link
Owner

Choose a reason for hiding this comment

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

Looking good!

lib/schema.json Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

I think we don't need this for now. Just removed it from design.md

@drom
Copy link
Author

drom commented Aug 10, 2015

How about a method on pipeline object like p.validate() that would do pipeline validation.

@indutny
Copy link
Owner

indutny commented Aug 10, 2015

@drom what exactly should it validate?

@drom
Copy link
Author

drom commented Aug 10, 2015

for the start it should call tv4.validate() to validate current state of the p object for the schema compliance. But we can extend it with formal graph connectivity checks.

@indutny
Copy link
Owner

indutny commented Aug 10, 2015

@drom p has more high-level representation, it can't be validated against JSON Schema.

@indutny
Copy link
Owner

indutny commented Aug 10, 2015

@drom I agree about graph correctness, though. There is no place where irreducible loops are supported at the moment. It would be great to check this.

@drom
Copy link
Author

drom commented Aug 10, 2015

@indutny about p object. I understand that we have to call p.render('json') under the hood to get the object that we can validate against the schema.

@indutny
Copy link
Owner

indutny commented Aug 10, 2015

Yeah, exactly :) I think it might be a good idea to validate it in tests, but exposing function sounds unreasonable.

@drom
Copy link
Author

drom commented Aug 10, 2015

@indutny for the graph topology part. Out of the list below; let me know what properties would you like to check, and what bullets are correct by construction:

  1. all links are referencing an object.
    1. nodes[].inputs
    2. nodes[].control
    3. cfg.blocks[].node
    4. cfg.blocks[].nodes[]
    5. cfg.blocks[].successors[]
  2. graph is 'connected' (the is no unreachable vertices)
    1. nodes
    2. cfg
  3. detect irreducible loops in CFG
  4. detect all loops in nodes[].input

@indutny
Copy link
Owner

indutny commented Aug 10, 2015

I think all links are surely referencing the blocks and nodes. This is guaranteed by JSONFormat parser.

Other points make sense! Thanks!

@drom
Copy link
Author

drom commented Aug 10, 2015

@indutny can we assume that all nodes are topologically sorted?

@indutny
Copy link
Owner

indutny commented Aug 10, 2015

@drom nope :)

@indutny
Copy link
Owner

indutny commented Aug 10, 2015

We have .reindex() for this.

@drom
Copy link
Author

drom commented Aug 10, 2015

@indutny both loops and irreducible loops can be detected during toposort. Do you perform it anywhere in the flow?

@indutny
Copy link
Owner

indutny commented Aug 10, 2015

It is an optional step, might be performed if some of the stages needs it (like linearscan).

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.

2 participants