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

Support for clojure.datafy #468

Closed
cldwalker opened this issue Jun 11, 2020 · 22 comments
Closed

Support for clojure.datafy #468

cldwalker opened this issue Jun 11, 2020 · 22 comments

Comments

@cldwalker
Copy link
Contributor

cldwalker commented Jun 11, 2020

Hi. Exciting that defprotocol support is out! I'm opening this issue to track support for clojure.datafy.

Soon after defprotocol support landed on master, I tried requiring clojure.datafy and ran into missing java classes e.g. Unable to resolve classname: java.lang.Iterable [at clojure/core/protocols.clj, line 34, column 6]. Here's what I tried to remedy missing java classes:

EDIT:
I'm able to require clojure.datafy with this branch. This branch adds all the java classes that namespace required. However when I try to exercise datafy functionality via clojure.datafy/datafy or clojure.datafy/nav, I don't see the expected behavior. To reproduce what I'm seeing, run the built ./bb of the above branch against this file e.g. bb -f explore_data_nav.clj. Observe that nav-to-link starting with this invocation returns a number instead of an expected map.

It's unclear to me if I'm adding java classes in the wrong places, if there's a sci issue with defprotocol or something else. The above failing example only navigates maps. Any thoughts?

Is your feature request related to a problem? Please describe.
Would like to use clojure.datafy for navigating clojure data and java classes because of its first class reach in the language

Describe the solution you'd like
Would like require clojure.datafy to work

Describe alternatives you've considered
There isn't another that has the same functionality and is built into the language

Additional context
n/a

@borkdude
Copy link
Collaborator

@cldwalker :extend-via-metadata isn't supported yet in defprotocol. I'll make a new sci issue for that.

@borkdude
Copy link
Collaborator

borkdude commented Jun 11, 2020

Added :extend-via-metadata to bb master now.

@borkdude
Copy link
Collaborator

@cldwalker I think clojure.datafy has to be added as a built-in namespace (either in sci or bb) due to some differences of what can be datafied (e.g. a sci namespace vs a clojure.lang.Namespace).

What would be some useful example of having this in bb?

@djblue
Copy link
Contributor

djblue commented Aug 10, 2020

Hi @borkdude, I might have a use case. I was thinking of adding support for the babashka runtime to portal. So far I have a basic http-socket-server setup, but things started to fail when I pulled in clojure.datafy. Let me know if there is anything I can do to help.

@borkdude
Copy link
Collaborator

@djblue I think this has to be built into the Clojure interpreter itself:

babashka/sci#355

It's on my list of things to look at, but feel free to experiment with it.

@cldwalker
Copy link
Contributor Author

cldwalker commented Aug 11, 2020

@borkdude Sorry for the delay. My use case is that have a knowledge graph that I'm able to navigate with REBL. I'd like to build a bb CLI that improves on REBL by being more extensible. I'm only navigating basic data structures e.g. maps, sets and vectors so I'm not particularly concerned if there's no support for namespaces or vars initially. Another use case I could see for datafy is being a a helpful documentation reference for any java class or clj data structure e.g.:

$ echo "(require '[clojure.datafy :as d]) (->> Iterable d/datafy :members vals (map first) clojure.pprint/print-table)" |clj                                                                               
Clojure 1.10.2-alpha1
user=> nil

|       :name |          :return-type |   :declaring-class |              :parameter-types | :exception-types |               :flags |
|-------------+-----------------------+--------------------+-------------------------------+------------------+----------------------|
|     forEach |                  void | java.lang.Iterable | [java.util.function.Consumer] |               [] |           #{:public} |
|    iterator |    java.util.Iterator | java.lang.Iterable |                            [] |               [] | #{:public :abstract} |
| spliterator | java.util.Spliterator | java.lang.Iterable |                            [] |               [] |           #{:public} |
nil
user=>

I updated the first comment with a branch that is able to require clojure.datafy but I'm unable to observe working functionality. I don't know when I'll dig into this more but thought it'd be helpful to share progress

@borkdude
Copy link
Collaborator

borkdude commented Aug 11, 2020

I added a branch datafy in this repo where I mounted the clojure.datafy namespace.

So this works:

$ ./bb "(require '[clojure.datafy :as d]) (d/datafy (atom 1))"
[1]

But somehow memory usage during compilation spikes and the binary gets quite bloated (95MB). I would like to see more details about why this happens, before continuing.

It could be related to the implementations of datafy for clojure namespaces, etc. Maybe including a patched version of clojure.datafy in babashka makes sense. I think I saw the same issue while trying to include spec.

@borkdude
Copy link
Collaborator

This is pretty weird. Just including a reference to (p/datafy ..) results into this bloat.

This is the patched namespace I was using:
https://github.com/borkdude/babashka/blob/694fb1b4273780e31412d699c3c920d22d2fb6f0/src/babashka/impl/clojure/datafy.clj

@borkdude
Copy link
Collaborator

This could be explained by some other dep loading clojure.datafy:

user=> (binding [clojure.core/*loading-verbosely* true] (require '[clojure.datafy]))
nil
user=> (keys (:impls clojure.core.protocols/Datafiable))
(nil java.lang.Object java.lang.Throwable clojure.lang.IRef clojure.lang.Namespace java.lang.Class)

@borkdude
Copy link
Collaborator

Discovered that the dep loading clojure.datafy was clojure.reflect which was loaded by core.async.

Commit 82016da in branch datafy:

$ export BABASHKA_FEATURE_CORE_ASYNC=false
$ script/uberjar && script/compile

This yields a normal sized binary. More research needed. Maybe we can patch clojure.reflect and/or clojure.datafy itself on our classpath, so no matter who loads it, they will have our version.

@borkdude
Copy link
Collaborator

Turns out that clojure.reflect isn't the problem, it's the Datafiable for clojure.lang.Namespace that gets a bloated binary.

@borkdude
Copy link
Collaborator

Made a PR here:

https://github.com/borkdude/babashka/pull/526/files

Could you both please check if babashka with this addition is working for your requirements?

@borkdude
Copy link
Collaborator

borkdude commented Aug 11, 2020

$ ./bb "(require '[clojure.datafy :as d]) (d/datafy Exception)"
{:bases #{java.lang.Throwable}, :flags #{:public}, :members {java.lang.Exception [#clojure.reflect.Constructor{:name java.lang.Exception, :declaring-class java.lang.Exception, :parameter-types [java.lang.String java.lang.Throwable], :exception-types [], :flags #{:public}} #clojure.reflect.Constructor{:name java.lang.Exception, :declaring-class java.lang.Exception, :parameter-types [java.lang.String], :exception-types [], :flags #{:public}} #clojure.reflect.Constructor{:name java.lang.Exception, :declaring-class java.lang.Exception, :parameter-types [java.lang.Throwable], :exception-types [], :flags #{:public}} #clojure.reflect.Constructor{:name java.lang.Exception, :declaring-class java.lang.Exception, :parameter-types [], :exception-types [], :flags #{:public}}]}, :name java.lang.Exception}

@borkdude
Copy link
Collaborator

@cldwalker Your example seems to work (minus the fact that Iterable isn't yet available in bb):

$ ./bb "(require '[clojure.datafy :as d]) (->> String d/datafy :members vals (map first) clojure.pprint/print-table)"

|                  :name |                :type | :declaring-class |                      :flags |
|------------------------+----------------------+------------------+-----------------------------|
| CASE_INSENSITIVE_ORDER | java.util.Comparator | java.lang.String |   #{:public :static :final} |
|                 charAt |                      | java.lang.String |                  #{:public} |
|                  chars |                      | java.lang.String |                  #{:public} |
|            codePointAt |                      | java.lang.String |                  #{:public} |
...

borkdude added a commit that referenced this issue Aug 11, 2020
@borkdude
Copy link
Collaborator

Part 1 is merged: support for clojure.datafy namespace. Implementing the clojure.core.protocols/Datafiable protocol within bb scripts will be part 2.

@borkdude
Copy link
Collaborator

borkdude commented Aug 11, 2020

In the datafiable branch:

$ ./bb "(require '[clojure.core.protocols :as p] '[clojure.datafy :as d]) (extend-type Number p/Datafiable (datafy [n] {:number n})) (prn (p/datafy 1)) (prn (keys (d/datafy *ns*)))"
{:number 1}
(:name :publics :imports :interns)

There's still a number of TODOs:

@cldwalker
Copy link
Contributor Author

@borkdude This is all exciting work! Thanks for getting part 1 in. This example seems to works for me now

@cldwalker
Copy link
Contributor Author

I confirmed with the datafiable branch I'm able to navigate my data structures as in clj! I wasn't able to in master due to failing to require clojure.core.protocols

borkdude added a commit that referenced this issue Aug 12, 2020
@borkdude
Copy link
Collaborator

Pushed final commits to the datafiable branch.

@djblue
Copy link
Contributor

djblue commented Aug 12, 2020

@borkdude Looks good to me. I was able to nav the hacker news api in babashka 🎉

@cldwalker
Copy link
Contributor Author

Latest datafiable branch works great with my knowledge graph! 🕺

@borkdude
Copy link
Collaborator

Merged!

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

No branches or pull requests

3 participants