-
Notifications
You must be signed in to change notification settings - Fork 23
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
Contributing to chainer-compiler #101
Comments
Thanks for offering help! Honestly, I'm not sure we are ready to accept contributions, but let me try suggesting some good first tasks. I think the current ONNX does not have
As for HardSigmoid, I think this op can be implemented by other ONNX ops. For such cases, I'm trying to use compiler/simplifier.cc to keep the backend implementation simple. 7505e17 could be a good example. As for the latter two ops, I think you need to define new XCVM ops and call ChainerX ops in the backend implementation. See 2cc5b43 for an example. As for CH2O/elichika: since we're thinking of switching to elichika from CH2O, we'd like more ops support in elichika. The process would be something like
However, currently, adding a new op is as simple as CH2O. I'm guessing we need to refactor how operator emitters are implemented in future, so I'm not very sure if adding new ops to elichika with the current interface is a great idea. @durswd, please correct me if I'm wrong. |
Thanks for the detailed information! I will start with the ops you mentioned. |
Sorry I forgot mentioning onnx-chainer. C++ chainer-compiler is an ONNX importer while onnx-chainer, CH2O, and elichika are ONNX exporters. For end-to-end user experience (export chainer model as an ONNX file and run it by chainer-compiler), adding new ops to onnx-chainer is important. So contributions are welcome. As you mentioned, chainer/onnx-chainer#44 will be a good example, though API was changed a bit since then. However, I'm not sure if there is a good easy op to be implemented. ONNX-chainer has a longer history than this project and it's coverage is fairly good. FYI, From perspective of C++ chainer-compiler, when you add support of an op, we need a test ONNX model which contains the op. The easiest way is to use ONNX's standard tests. For three ops I mentioned, ONNX has test cases (https://github.com/onnx/onnx/tree/master/onnx/backend/test/data/node/test_hardsigmoid). You can find it in third_party/onnx/onnx/backend/test/data/node/test_hardsigmoid in your cloned repository. When there is no ONNX's standard testcase (e.g., non-standard ops) or it's coverage is not 100% (e.g., backprop), we need to generate tests by ourselves. For such case, we use scripts/gen_backprop_tests_oc.py (onnx-chainer based test generator), scripts/gen_backprop_tests_pc.py (ch2o based test generator), or scripts/gen_extra_tests.py. |
Hi @shinh Since ch2o is an ONNX exporter, from what I have understood, I think that the func.py file contains a mapping from Chainer functions to ONNX ops. While it seems straightforward to implement those functions which have a similar counterpart in ONNX ops (such as F.expand_dims -> unsqueeze), I couldn't comprehend ONNX op names such as 'ChainerSequenceStack' or 'ChainerResizeImages' and how they are themselves defined. |
We added some custom ops to ONNX. I think they are either
I think we should/want to propose some ops in the category 3, but unfortunately, we haven't done yet :( Though there's no documentation about these custom ops, here is the list: https://github.com/pfnet-research/chainer-compiler/blob/master/compiler/gen_node.py#L212 Also, note other vendors have some custom ops (e.g., https://github.com/Microsoft/onnxruntime/blob/master/docs/ContribOperators.md). |
@vermashresth Please let me hijack this issue to make sure information is not scattered. Questions from Rishav:
I think the former question is mostly answered in #101 (comment) As for major libraries, I think you first need to understand ONNX. I think these two documents are very helpful to familialize yourself to ONNX. https://github.com/onnx/onnx/blob/master/docs/IR.md
may be hints to help you to understand how operators are created and executed. |
@shinh Thank you for addressing my questions. I find myself interested in extending the processing of python jump statements in both elichika and ch2o. I am aware of how dreadful using tf.while_loop with break condition is. Extending onnx graph generation to support loops with jumps would be an awesome feature I think. I have some more queries relating to elichika and ch2o.
|
Yeah, my experience with it was exactly the motivation I wanted Python loop support in the compiler stack.
Yes, you are right. (ch2o|elichika)/tests/node should be almost accurate. You would see elichika really needs more ops :)
Both CH2O and elichika do not emit graphs with backprop. The compiler middle end generates op nodes for backward computation. (https://github.com/pfnet-research/chainer-compiler/blob/master/compiler/gradient.cc and https://github.com/pfnet-research/chainer-compiler/blob/master/compiler/gradient_ops.cc) The commented out code was meant to generate expected gradient values by running the test model with Chainer. It was just a TODO, and it was fairly easy to fix: #124
I'm not 100% sure, but I think you are right (@durswd would correct me if I'm wrong). In general, I think there are a bunch of rooms for improvement around name lookup in elichika. Patches are welcome :)
Yes, you are right. It should be very easy to add ops once we implemented basic framework to handle variety of ops. As for the specific case of
Also, in addition to inputs/outputs, you also need to set attributes properly. For example, ONNX's |
@shinh I have the a doubt regarding ForAndIf.py syntax test in ch2o. It seems from the netron visualization of the generated onnx file that the if condition computation graph is not being represented. I even tried the official net drawer tool, and it seems it also doesn't completely represent the graph (though does give more details). I want to see the details of loop subgraph. Is there a way to do so? Also, what does _find_in_out() function do exactly? I understand that it is keeping track of values will change inside the for loop body. But there are very specific condition based statements inside that I an unable to comprehend. |
I think these visualizers do not support subgraphs, unfortunately. Usually, I just read ASCII proto dump from |
Yeah, |
Hi @shinh
I have built chainer-compiler from source and have been going through the code base. I would like to contribute but needed some directions to start.
For adding Chainer Functions support ( like Issue #67 ), should the workflow first include adding the support for the function in onnx-chainer ( like PR chainer/onnx-chainer#44) using ops provided by ONNX or can they be implemented as done in PR #62 ?
Moreover, I am also interested in working with the Python AST for ch2o and elichika. Could you please suggest some basic feature improvements that I could try working on to get an idea about it?
The text was updated successfully, but these errors were encountered: