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

proof of concept for attaching :doc metadata to generate functions #98

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

behrica
Copy link
Member

@behrica behrica commented May 7, 2024

This is very draft.
I was not able to re-use the existing "help" , and need a hack to have a session.
And on this I am not sure, how to do better.

@behrica
Copy link
Member Author

behrica commented May 7, 2024

@genmeblog, maybe you can improve on it.

@behrica
Copy link
Member Author

behrica commented May 8, 2024

I removed the duplication of the help logic.
Not 100% sure, if I pass the "session" around correctly.
But it does work now.

BUT its slow.
It increases quite a lot the time of require-r, not sure if something can be done.
I noticed as well that "Calva" is not able to render tgis type o dynamic ":doc".
Emacs does, and repl shows them as well yuing (doc ...)

@behrica
Copy link
Member Author

behrica commented May 8, 2024

Can be used like this:

(reset! clojisr.v1.require/attach-help-as-docstring-to-vars true)
(r/require-r '[ggplot2])
(r/require-r '[ranger])
(r/require-r '[base])

Help generation is as well memoized, so at least fast on second call

@behrica behrica requested a review from genmeblog May 8, 2024 11:36
@behrica
Copy link
Member Author

behrica commented May 8, 2024

It does work as well for Calva, not sure why it failed as I tried.

@genmeblog
Copy link
Member

Maybe instead of attach-help-as-docstring-to-vars add an option to require-r itself?

(require-r '[ggplot :docs? true])

@behrica
Copy link
Member Author

behrica commented May 8, 2024

Maybe instead of attach-help-as-docstring-to-vars add an option to require-r itself?

(require-r '[ggplot :docs? true])

Yes, I thought about it.
Not sure, what best.

Ideally we could speed it up..., but likely not possible.
To mee it something you "either want" or "don't want".

Not per library.

@genmeblog
Copy link
Member

Yes, you're right, so maybe an argument to require-r itself?

@behrica
Copy link
Member Author

behrica commented May 10, 2024

FYI:
Currently there is a bug in VSCode / Calva which prevents the rendering of this help attached to the vars.
BetterThanTomorrow/calva#2552
But it seems fixable, as Calva has already foreseen to take doc from Cider and not only from LSP.
LSP is not informed about such dynamic vars, so cannot report this doc strings.

@behrica
Copy link
Member Author

behrica commented May 11, 2024

In my view the "slow R package loading" due to help loading is acceptable, as it happens only once per R package, then its cached.
So maybe an atom to switch it on/off is enough.
Or we make it "delayed", So we do an "immediate loading" without help,
and the we load again on an other thread in a background.

@genmeblog
Copy link
Member

We can also alter vars in the background I think.

@behrica
Copy link
Member Author

behrica commented May 12, 2024

I am pretty happy with it now.

The :doc gets now changed to be "help" via alter-meta! in a future, always.
This is transparent for the user in most cases, as in repl the require returns immediately.
and a few seconds later, the doc string of the var is updated, and IDEs should show it.

@behrica
Copy link
Member Author

behrica commented May 12, 2024

Remark: as we use now a "future" we get the 60 seconds waiting when clojure shuts down

@behrica
Copy link
Member Author

behrica commented May 12, 2024

time clj -e "(require '[clojisr.v1.r :as r])(r/require-r '[utils])"

real    1m10.797s
user    0m29.738s
sys     0m0.844s

Here we see the 60 seconds delay.

vs

time clj -e "(require '[clojisr.v1.r :as r])(r/require-r '[utils])(System/exit 0)"
real    0m14.728s
user    0m29.386s
sys     0m0.827s

same duration as:

time clj -e "(require '[clojisr.v1.r :as r])(r/require-r '[utils])(shutdown-agents)"

real    0m15.212s
user    0m29.055s
sys     0m0.767s

So people doing scripts just need to remember to add it.

src/clojisr/v1/require.clj Outdated Show resolved Hide resolved
src/clojisr/v1/require.clj Outdated Show resolved Hide resolved
src/clojisr/v1/require.clj Outdated Show resolved Hide resolved
src/clojisr/v1/require.clj Outdated Show resolved Hide resolved
test/clojisr/v1/help_test.clj Outdated Show resolved Hide resolved
@behrica behrica requested a review from genmeblog September 22, 2024 21:49
@behrica
Copy link
Member Author

behrica commented Jan 14, 2025

The latest change removes the future, and uses (.start (Thread ,,) instead.
This feels still fast when requiring the r library
You can try with these, which are big (= lots of function):

(r/require-r '[base])
(r/require-r '[stats])

@behrica behrica requested a review from genmeblog January 14, 2025 21:02
@behrica
Copy link
Member Author

behrica commented Jan 15, 2025

The 60 second problem is already present on "main". Once we start the R session in any form,
the JVM takes 60 second to stop.

So it is not related to any changes I did in this PR.

@genmeblog
Copy link
Member

Maybe this is the reason?

https://github.com/scicloj/clojisr/blob/master/src/clojisr/v1/r.clj#L201-L203

Are you able to verify this by turning this off for tests (I can't do this now)?

(add-to-ns ns-symbol r-symbol r-object))
(run!
(fn [[r-symbol r-object]]
(assoc-doc-to-meta! ns-symbol r-symbol r-object))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In rich packages you'll run zillions of threads. The whole loop should be run in one thread.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried this before, in a future.
That introduced a "noticeble lack" when evaluating:
(r/require-r '[base])
(r/require-r '[stats])

(unless we put a Thread/sleep in the beginning. See comments above)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a trick.....
So we do the slow reading of the help 'on demand'

(defn- assoc-doc-to-meta! [ns-symbol r-symbol r-object]
  (alter-meta!
   (get (ns-publics ns-symbol) r-symbol)
   assoc :doc (reify Object
                (toString [this] (safe-help r-object)))))

This works in vscode.
the value of the :doc is now not a string, but a function , implementing "toString".

Copy link
Member Author

@behrica behrica Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked about this on slack, let's see.

Copy link
Member Author

@behrica behrica Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a good idea it seems, and I somehow agree.

Docstrings are supposed to be strings, that's it.
Even if all the tools that you use accept non-strings, relying on it would be wrong.
The ns macro explicitly checks for string?, as does defprotocol.
find-doc works only on strings and would throw on anything else.
And so on.
There are four alternatives that I see:
Just tolerate the loading time
Make every docstring the same, and in it mention that a user should call something like generate-docs that would take some time and update run-time docstrings
Same as the above, but instead of generate-docs it would be custom-doc that retrieves the docstring just for the provided function
If the R libraries are known in advance, generate Clojure code from them, along with all the docstrings

@behrica behrica requested a review from genmeblog January 15, 2025 13:56
@behrica
Copy link
Member Author

behrica commented Jan 15, 2025

In last commit I came back to your original proposal of adding an option to require-r, by default "false".
Then we can document it as being slow.

@behrica
Copy link
Member Author

behrica commented Jan 15, 2025

My last change breaks:
(r/require-r '[graphics :refer [plot hist]])

@behrica
Copy link
Member Author

behrica commented Jan 15, 2025

Ok, I made a new try of a parameter 'generate-doc-strings?', default to false

@genmeblog
Copy link
Member

My last change breaks: (r/require-r '[graphics :refer [plot hist]])

Does it work now?

@behrica
Copy link
Member Author

behrica commented Jan 16, 2025

My last change breaks: (r/require-r '[graphics :refer [plot hist]])

Does it work now?

yes

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

Successfully merging this pull request may close these issues.

2 participants