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

New version of the vtk package #998

Merged
merged 66 commits into from Mar 23, 2015
Merged

New version of the vtk package #998

merged 66 commits into from Mar 23, 2015

Conversation

rexissimus
Copy link
Member

This is finished, but could use some more testing.

  • Works on both VTK6 and VTK5!
  • Existing workflows should still work.
  • Uses a general python class wrapper.
  • A wrapper class handles overloaded patches.
  • A wrapper spec is generated the first time the package is loaded.
  • The spec is stored in the .vistrails folder as vtk-spec-VERSION.xml.
  • Package loading time reduced from ~25s to ~10s (except first time).
  • VTK6 style port names (e.g. SetInputData) are used even on VTK5.
  • Most Set/Get port prefixes have been removed.
  • SetOn/SetOff methods are now booleans.
  • SetXToY enum methods are now real enums.
  • No more empty functions (they are now booleans).
  • VTKCell is now used in place of VTKViewCell (fixes Remove vtkviewcell #1019).
  • vtkMultiBlockPlot3Dreader replaces vtkPLOT3DReader.
  • Fixes VTK file loading on non-english locales.
  • Uses new-style package upgrades.
  • Most ports now use port depth 1 and can be connected to lists of objects.

Still very much in progress, main work is on improving the port specs
and specifying them explicitly. vtk_classes.py is huge due to
docstrings and tons of classes.  A simple head example works!
@remram44
Copy link
Member

I did a very quick toy example: https://gist.github.com/remram44/f85405f6473a326ad4a3

A priori, loading Python code is way slower (0.07s) than JSON+dynamic creation (0.03s), although loading compiled bytecode (.pyc) is faster (0.02s). But all this is very comparable, so insertion into the registry will probably be the bottleneck.

@rexissimus
Copy link
Member Author

Cool, so dynamic creation should not be a problem.

The intermediate format is currently xml. We could use either json, xml, or python classes as a final representation. @dakoop worries about readability when debugging, so this will be a tradeoff between parsing speed and readability. Maybe some well-structured json would not be too bad?

If we keep the python classes where should it be stored? It will be generated the first time the package is loaded and when the vtk version changes?

Adding @dakoop's previous comments:
About the VTK wrapping, I see Rémi's point about using the intermediate representation directly. This should be ok, but will debugging suffer? Also, one thing I would then like to see is the ability to have some type of patterns/shortcuts for patching and/or extensions. For example, the matplotlib stuff has an idea of alternatePortSpecs that are used to define two versions of a port (one scalar, one list); those dictate both a second port spec and logic for handling this in the compute method. Also, should we then look to have the XML mirror the registry specifications? (My vote: yes, but need to have some way to instrument extensions--second intermediate?) Should this go into the schema? (My vote: probably yes but wouldn't spend too much time on this).

@rexissimus
Copy link
Member Author

@dakoop are you suggesting a common XML(or json?) for all wrappers where there is a core that specifies e.g. name/function/inputs/outputs and an extension section that specifies things like alternatePortSpecs?

Extensions could be passed to the registry but is it not easier to add it to the dynamically created module? Is that what you mean by the XML mirroring the registry specifications?

For patching, does this mean adding/removing/changing/correcting things in the intermediate representation using different patterns? This should be easy to code by hand but we could have methods for doing common tasks?

@dakoop
Copy link
Member

dakoop commented Jan 29, 2015

@rexissimus, if we have a standard intermediate representation (json/xml), my argument is that it should mirror the registry schema (which defines db classes for packages, module descriptors, and port specs). The matplotlib xml representation differs from the registry schema because it contains items that affect the automated compute method construction as well as the structure of the package. Such items are useful for patching as well; it is much easier for me to create an alternatePortSpec than to make manual changes to both the module inputs/outputs and compute method. Note the matplotlib package requires compute methods that are not simple maps from input ports -> input parameters and output values -> output ports. Thus, for that package (and perhaps others), I would need extensions to the proposed intermediate representation that would allow rules that impact both the specification and the compute method. Or, I could use a secondary intermediate specification where those edits could be made and the compute methods updated. In either case, we need some way to specify what the compute method should look like in the intermediate representation. The compute methods can be specified by recipes that are templates (perhaps mako which I've used elsewhere in the code).

@rexissimus
Copy link
Member Author

With dynamic wrapping you can still do that. The compute method would be shared, like gen_module_ufunc. The alternatePortSpec would be stored as an extension and used for both specification and as input to the compute method when the module is dynamically created. I just don't see the advantages of your way?

@dakoop
Copy link
Member

dakoop commented Jan 29, 2015

@rexissimus, I am missing something---is there a proposed architecture that should be linked to this PR? Could you clarify what "my way" is; is this the matplotlib/vtk wrapping method? I am fine building off of the VTTools or other infrastructure, and the main concern I have with dynamic generation is debugging. I also want to make sure that whatever is adopted is general enough to work for something like the matplotlib wrapping. Some questions:

  • What do you mean by the compute method being shared? What happens if I want to specify different compute methods for different types of modules?
  • What do you mean by an extension---is there a proposed extension syntax?
  • How does debugging work for dynamically created compute methods?

@rexissimus
Copy link
Member Author

I meant the way you want to make the intermediate representation similar to the registry schema. I am trying to understand how that would work.

  • In VTTools one compute method is created for each module, but you could also use a single compute method that uses the intermediate module representation (stored in a module attribute) to figure out which inputs/outputs to use and which function to execute. I am not sure which method would be preferred. To make the compute method less complex you could have different compute methods fir different types of wrapped modules.
  • With extension I mean how non-standard attributes are stored in the intermediate representation. Actually, | am not sure there would be many standard attributes except e.g. name and namespace. The function/class to execute can be specified in different ways, and ports can be specified in more complex ways like with the "AlternatePortSpec".
  • I am not sure debugging would be a problem since you still have an explicit compute method. It would just be the same for a group of modules, but you could look at the attributes to figure out which one it is?

Conflicts:
	vistrails/packages/vtk/identifiers.py
	vistrails/packages/vtk/init.py
	vistrails/packages/vtk/tf_widget.py
	vistrails/packages/vtk/vtkcell.py
@rexissimus
Copy link
Member Author

Ok, I think I see your point now after working with the code. The intermediate representation should mirror the registry so that it is easy to see what the final module will look like. Patching will affect both the specification and the shape of the final compute method that would have to be specified by some kind of extension.

@tacaswell
Copy link
Contributor

@dakoop I would also point out I have been thinking about how to coerce matplotlib to fit in better with a the functional nature of VT (which involves an Axes class which doesn't add the artists to it's self so you can collect them all and then add them en-mass to an Axes is the last plotting step instead of collecting code to be run) which will make the mapping of input/output ports much closer to args/return values.

Modules are now loaded directly from vtk/vtk.xml

The package loads but some patterns are not working yet
* VTK classes are dynamically wrapped as python functions that are
  independent of vistrails
** Function output can be single, list, or dict
** spec describes how to wrap function into a vistrails module
** module wrapper can be used with any python function
** Exporting to script could convert modules into the wrapped functions.
TODO:
* Auto-generate xml and store somewhere
* Inspectors
* Make VTKCell optional
* Upgrades

VTK classes are wrapped into functions that have some magic attributes:
* _callback(float) - used to report progress to logger
* _tempfile - function for creating tempfile (for using FilePool)
* _outputs - list of connected output ports for optional output generation
This is needed to make sure data files are parsed
correctly on non-C locales.
VTK is wrapped using a spec describing how to transform
a vtk class into a vistrails module

New Class wrapper handles VTK style Set/Get classes where class
instanciation takes no arguments

* All VTK examples works
* TODO: Refactor general classes to vistrails core

generate/parse.py: Removed arg usage
generate/specs.py: New specs for python classes and python functions
init.py: New Class wrapper
inspectors.py: Fixed missing attribute
vtk_classes.py: Port logic moved to class wrapper

deleted:
  base_module.py
  generate/vtk.xml
  generate/vtk_raw.xml
core\wrapper - contains code for executing wrapped objects
package\vtk\vtk_wrapper - a vtk class wrapper independent of vistrails

new: common.py - common functionality for wrapper types
new: core/wrapper/pythonclass.py: code for executing a python class
new: core/wrapper/pythonfunction.py: code for executing a python function
renamed: generate/specs.py -> ../../core/wrapper/specs.py
new: vtk_wrapper/specs.py: Placeholder for core/wrapper/specs.py
renamed: generate/class_tree.py -> vtk_wrapper/class_tree.py
renamed: fix_classes.py -> vtk_wrapper/fix_classes.py
renamed: generate/parse.py -> vtk_wrapper/parse.py
renamed: vtk_classes.py -> vtk_wrapper/vtk_classes.py
renamed: generate/vtk_module.py -> vtk_wrapper/vtk_module.py
renamed: generate/vtk_parser.py -> vtk_wrapper/vtk_parser.py
moved:   vtk -> vtk_wrapper/wrapper.py
deleted:    class_tree.py
deleted:    generate/generate.py
deleted:    generate/mixins.py
deleted:    generate/vtk_template.py.mako
deleted:    vtk_parser.py
All vtk examples works!

Fixed vtkPLOT3DReader upgrade to MultiBlockPLOT3DReader
Using custom output port StructuredGrid to get normal output

Added SetInputData-style support

TODO: Make sure files can be opened on both VTK5 and VTK6
by using Set*Data on VTK6 and Set* on VTK5

init.py
vtk_wrapper/parse.py
vtk_wrapper/vtk_classes.py
.vt files should now work on both VTK5 and VTK6.
The new VTK6 SetInputData-style names are always used now.

TODO: This needs more testing
All vtk_examples now work on both VTK5 and VTK6

init.py:
  Added more upgrades for Set and overloads

vtk_wrapper/parse.py:
  Removed more Set prefixes
  Fixed VTK5 name upgrades

vtk_wrapper/vtk_classes.py: Fixed comment
Conflicts:
	vistrails/packages/vtk/base_module.py
	vistrails/packages/vtk/init.py
	vistrails/packages/vtk/vtk_wrapper/vtk_parser.py
	vistrails/packages/vtk/vtkviewcell.py
This was referenced Mar 18, 2015
@remram44 remram44 added this to the version 2.2 milestone Mar 19, 2015
Conflicts:
	vistrails/packages/vtk/base_module.py
This is for including it without exposing the wrapper api
rexissimus added a commit that referenced this pull request Mar 23, 2015
New vtk package with the wrapping code bundled
@rexissimus rexissimus merged commit f250728 into master Mar 23, 2015
@remram44 remram44 mentioned this pull request Mar 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants