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

Concept code for dependency management and custom pipelines #75

Closed
wants to merge 1 commit into from

Conversation

lasalvavida
Copy link
Contributor

@lasalvavida lasalvavida commented Jun 10, 2016

Establishes a way for specifying dependency hierarchies and shows how the configuration would look for quantizeAttributes().

@pjcozzi, @lilleyse, could you take a look at this when you get a chance and tell me what you think?

Pipeline stages put their dependencies in a JSON file in lib/dependencies that matches their name.
You can then call runStages() with an Array of stage names and it will run them in order accounting for pre and post dependencies. The default.json configuration in bin is an example of a pipeline configuration that specifies a list of stages.

@JoshuaStorm, your input here will also be valuable in terms of finishing the implementation.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 93.862% when pulling f3679d3 on json-dependencies into 223b707 on master.

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Jun 10, 2016

This is going to require some thought for tests. Do we explicitly call necessary dependency functions in the specs, like I did here, or do we split out runStage from runStages and call that in the specs?

edit: something like

var quantizeAttributes = function(gltf, options) {
    runStage(gltf, 'quantizeAttributes', {}, {}, [], options);
};

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 10, 2016

To make development easier and improve cohesion, it would be better to group the before/after dependencies in the same file as each stage.

For example, instead of a stage being just a function, it can be a class that has a function to execute the stage and array properties for the before/after dependencies.

I can provide a more specific example if you need.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 10, 2016

I briefly looked at the implementation and it looks OK.

The two main things to carefully account for and test are:

  • Stage dependencies with their own dependencies...with their own dependencies...and so on.
  • Not executing the same stage more than once.
    • However, we may learn of cases where we actually want to do this, e.g., "compact buffers now even though it happens later because doing now makes the rest of the pipeline faster." Let's add support for this when we need it.

@lasalvavida
Copy link
Contributor Author

The two main things to carefully account for and test are:

  • Stage dependencies with their own dependencies...with their own dependencies...and so on.
  • Not executing the same stage more than once.
    • However, we may learn of cases where we actually want to do this, e.g., "compact buffers now even though it happens later because doing now makes the rest of the pipeline faster." Let's add support for this when we need it.

I think I actually did a pretty good job accounting for those cases already. Take a look at the included spec.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 10, 2016

history is only used for testing, right?

You should be able to remove it and use spies instead. @lilleyse can advise if you need help.

See http://jasmine.github.io/2.4/introduction.html#section-Spies

@lasalvavida
Copy link
Contributor Author

history is only used for testing, right?

It is, but it might be useful to rework this a bit so that it resolves dependencies, outputs the list of necessary stages in order (history currently), and then executes that.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 10, 2016

Ah, yes, that could be a nice implementation.

@JoshuaStorm
Copy link

What exactly is the expectation for producing the custom pipeline JSONs on the client side? Are we expecting users to just hand modify a JSON to set up a pipeline? Just want to make sure I am on the right page here.

Also, do we plan on setting up the pipeline so full configurations can be made via command line (without JSON configuration)? I imagine that would become quite cumbersome on the user

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 13, 2016

Are we expecting users to just hand modify a JSON to set up a pipeline?

Yes. End users of the command-line tool would do this.

Node.js developers would also call a top-level pipeline function and pass in the same JSON object.

Also, do we plan on setting up the pipeline so full configurations can be made via command line (without JSON configuration)? I imagine that would become quite cumbersome on the user

To start, I would say no. We could include a handful of typical pipelines in .json files.

Longer-term, if this is needed, PDAL has a pretty elegant approach. Did you review it?

@JoshuaStorm
Copy link

I did have a look at PDAL's approach, but wasn't really sure if that would be the best approach while the pipleline only has a handful of stages.

This was referenced Jun 16, 2016
@lilleyse lilleyse mentioned this pull request Jul 11, 2016
8 tasks
@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 31, 2016

Is it reasonable to come back to this and finish it off once the other outstanding PRs are merged?

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 5, 2017

This is superseded by #233. Thanks for looking into this, @lasalvavida.

@shehzan10 can you review this to see if anything is useful for #233?

@pjcozzi pjcozzi closed this Feb 5, 2017
@pjcozzi pjcozzi deleted the json-dependencies branch February 5, 2017 00:36
@ggetz ggetz added the cleanup label Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants