-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[Clojure] Add methods based on NDArrayAPI/SymbolAPI #14195
[Clojure] Add methods based on NDArrayAPI/SymbolAPI #14195
Conversation
@mxnet-label-bot add [Clojure, pr-awaiting-review] |
Thanks for looking into this. It will be interesting to see how it comes out. The new API might be more difficult to interop with than the regular data structures of the current one. |
@gigasquid it does seem like the new API is somewhat more cumbersome in terms of interop. the main benefit i see is the type checking but while in scala you can call the functions with named optional arguments (e.g. |
@kedarbellare Thanks so much for looking into this. I thought it might be the case that it might not be as nice to use. That's totally fine. We can stick to the other API for now and if for some reason we need to move off of it (like the Scala package deprecates it) , we can think of a different strategy, like moving a level down for NDArrary and Symbol and using the Scala Generator functions to generate our own functions directly using the JNI calls instead. For example - getting all the backend functions by calling the JNI and these to create the functions Let me know what you think. |
Makes sense. I'm working on unit tests for NDArray API right now which should give an idea of how we could start using it. I won't put too much effort into doing the same for Symbol API. It'll be great if we can start using JNI directly but I don't have much experience on that front. |
@kedarbellare Thanks for tackling the toughest part of the whole Clojure package 💯 I think you have made some really nice progress and we can definitely leverage some of this work. Since we are starting a new ndarray api, we have a nice opportunity to improve upon the current api and make it more useful for Clojure users. I agree with your assessment that the typing makes things better, but all the positional arguments and options make it more cumbersome. This is due to the philosophical differences between Clojure and Scala, where Clojure loves data structures and Scala loves types :) I'm thinking we should take a step back and think about what we (the clojure package) would want and then try to shape what you have into it. After looking at what you have, I think there is a path forward with maybe generating only the arity with all the parameters and then taking a data structure of vectors or maps to get there. I think it would be nice to outline what that API should look like with a couple examples from the NDArray and Symbol API before we code it up too much. Since you have experience and good feeling for what is possible now with your work, are you up for helping to draft up the future look of the API on the wiki? |
Yep. I wholeheartedly agree about the philosophical differences between Scala and Clojure and I completely see the value in doing what's best for the Clojure package. I can try to take a stab at creating an outline so that we can add it to the wiki and ask for comments. Note, however, that I mostly work on this project during the weekends or when I have free time so I'll try my best to put something together soon (over the next week or so). Please let me know if you want to have something out there ASAP, in which case it may not make sense for me to work on it. |
@kedarbellare Definitely no rush - whatever pace works well for you. I appreciate your efforts and your contributions to improve the Clojure project. I don't have rights to create accounts on the wiki so I'll ask out on dev for access for you. Thanks again! |
@kedarbellare You should have access now - ping me if you have any trouble 😸 |
@mxnet-label-bot update [Clojure, pr-work-in-progress] |
@gigasquid i've improved the api generation for both ndarrays and symbols. PTAL thanks! |
37e86b3
to
ffb73f8
Compare
Awesome work!!! I look forward to reviewing this 😸 |
contrib/clojure-package/test/org/apache/clojure_mxnet/symbol_api_test.clj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work Kedar!
I left some minor comments. I will review more in depth if needed. @gigasquid will have better feedback for you :)
Looking forward to having this merged!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kedarbellare for tackling this and making such great progress!
After @Chouffe 's feedback is taken care of, I think this is fine to merge and will give us a good base to work on.
contrib/clojure-package/test/org/apache/clojure_mxnet/symbol_api_test.clj
Outdated
Show resolved
Hide resolved
@kedarbellare - I think the next step after this is to try to get https://github.com/apache/incubator-mxnet/blob/master/contrib/clojure-package/test/org/apache/clojure_mxnet/conv_test.clj to use the new symbol-api. It will probably shake out some further refinements. It certainly is a much nicer api to use and would love to get it to the point were we could deprecate the old symbol and ndarray way of doing things 😸 |
I'm also not sure why the unix-gpu keeps failing from CI, might want to try to rebase on master and see if that helps |
Oh @kedarbellare - one more thing... Would you mind marking the new api namespaces as Thanks! |
@Chouffe @gigasquid : thanks for the detailed feedback! i'll take care of these issues soon (hopefully by end of the week). @gigasquid : where should i mark the namespaces as |
@kedarbellare marking it as Experimental in the namespace docstring of contrib/clojure-package/src/org/apache/clojure_mxnet/ndarray_api.clj - or a comment should be fine. And in the corresponding symbol-api namespace. Thanks! |
We should probably start rewriting some of the examples with this once it gets merged. |
3d2c01b
to
dc3a5fa
Compare
@gigasquid @Chouffe i've updated the PR with the following:
PTAL thanks! |
* [Clojure] Add methods based on NDArrayAPI/SymbolAPI * Add symbol API methods and ndarray API unit tests * Some more ndarray API unit tests * Explore direct use of JNI * Use library info directly instead of reflection * Add tests for generation op info * Fix ordering of keys using array-map * Ignore generated test files * Minor style changes * Refactor code for better readability * Address comments * Small tweaks to symbol api coercion
* [Clojure] Add methods based on NDArrayAPI/SymbolAPI * Add symbol API methods and ndarray API unit tests * Some more ndarray API unit tests * Explore direct use of JNI * Use library info directly instead of reflection * Add tests for generation op info * Fix ordering of keys using array-map * Ignore generated test files * Minor style changes * Refactor code for better readability * Address comments * Small tweaks to symbol api coercion
Description
Base/_LIB
with direct access to JNI in order to generate the ndarray and symbol methods along with named arguments and documentation. Although the current PR doesn't add specs these can be added over time.Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Reviewers
@gigasquid