scicloj / clojisr

Clojure speaks statistics - a bridge between Clojure to R
https://scicloj.github.io/clojisr/
Eclipse Public License 2.0
147 stars 9 forks source link

cljdoc build failed #94

Open behrica opened 1 month ago

behrica commented 1 month ago

I added cljdoc badge for Clojisr. Build is failing: https://cljdoc.org/builds/78095

Not sure, if easy fix

genmeblog commented 1 month ago

cljdoc tries to load a namespace and run a R session

INFO: [:clojisr.v1.session/make-session {:action :new-session, :id nil, :actual-session-args {:session-type :rserve}}]
{:clojure.main/message
 "Execution error (AssertionError) at clojisr.v1.impl.rserve.proc/r-path (proc.clj:30).\nAssert failed: (not= % \"\")\n",
 :clojure.main/triage
 {:clojure.error/class java.lang.AssertionError,
  :clojure.error/line 30,
  :clojure.error/cause "Assert failed: (not= % \"\")",
  :clojure.error/symbol clojisr.v1.impl.rserve.proc/r-path,
  :clojure.error/source "proc.clj",
  :clojure.error/phase :execution},
 :clojure.main/trace
behrica commented 1 month ago

Yes, noticed it. This line https://github.com/scicloj/clojisr/blob/82b4b77e53cc938c95694f19512c6b0187de3a5b/src/clojisr/v1/applications/plotting.clj#L14

fails on CI, correct ?

behrica commented 1 month ago

I will try to add "no-doc" metadata: https://github.com/cljdoc/cljdoc/blob/master/doc/userguide/for-library-authors.adoc#declaring-your-public-api Not sure, if it prevents ns loading.

behrica commented 1 month ago

It would only be visible on a new release, I believe. cljdocs build from maven jars, not from source. I propose we wait for next release and see if the cljdoc build works.

Its a nice way to have clojure docs for the API, so we should support it, I think

genmeblog commented 1 month ago

Yes, it fails on CI. There is no R in the environment. clojisr.v1.r also will try to run a backend.

https://github.com/scicloj/clojisr/blob/82b4b77e53cc938c95694f19512c6b0187de3a5b/src/clojisr/v1/r.clj#L127

behrica commented 1 month ago

I thought so....

behrica commented 1 month ago

Maybe its overkill just to solve this issue, but did you ever try to do these "def" lazy and via "intern"..

genmeblog commented 1 month ago

We were working on new session management system to solve this issue. But never finished. Maybe there is a way to refactor current setup without changing the API but I don't know it now. Any concrete ideas?

behrica commented 1 month ago

I got this working:

(defn r== [e1 e2] ((clojisr.v1.r/r "`==`") e1 e2))

This would avoid the def and so the ns loading should "do nothing". Not sure, this can be done for "all" def in the ns.

genmeblog commented 1 month ago

This way every call (r "==") will be reevaluated, RObject created and so on.

Anyway this is very good idea and I like it. We also get a function instead of var.

behrica commented 1 month ago

I tried as well for r$, works in same way. Are all these defs about "binary" functions ? Or as well functions taking any number of args ?

genmeblog commented 1 month ago

I don't know now. We have to go one by one. This strategy can be used also for plotting by replacing all RObjects calls into a string representation.

r.grDevices/dev-off -> "dev.off()" etc...

behrica commented 1 month ago

Not sure, it can fully work for Clojisr, but it would follow a "clojure principle" that ns loading should not do "anything".

behrica commented 1 month ago

I make a new issue at least , and link to this one.

behrica commented 1 month ago

I don't know now. We have to go one by one. This strategy can be used also for plotting by replacing all RObjects calls into a string representation.

r.grDevices/dev-off -> "dev.off()" etc...

Would this be needed ? Or just do the " (require-r '[grDevices]) " later , "inside" the plot function ? Maybe with a delay to avoid repeated calls ?

genmeblog commented 1 month ago

I don't know now. We have to go one by one. This strategy can be used also for plotting by replacing all RObjects calls into a string representation. r.grDevices/dev-off -> "dev.off()" etc...

Would this be needed ? Or just do the " (require-r '[grDevices]) " later , "inside" the plot function ? Maybe with a delay to avoid repeated calls ?

Yes, this is needed because later the symbol interned by require-r is used.