Fork me on GitHub
#babashka-sci-dev
<
2022-04-02
>
cap10morgan15:04:58

@borkdude Saw you merged the uberjar support PR. I'm happy to say more about the arg parsing fns refactor if you want me to. Still on my road trip but I get back to real life tonight.

borkdude15:04:19

Feel free to say more when you get back :) I'll try to get the uberscript stuff in too, so we can prepare for a release hopefully mid next week

šŸ‘ 1
borkdude15:04:32

@cap10morgan Did we implement caching for :path pods or not? If you're developing a pod in the JVM it can be pretty slow to load, and loading it an extra time takes a long time... I notice that when running tests :)

borkdude15:04:12

I noticed this takes pretty long on my machine:

(deftest uberjar-with-pods-test
  (testing "jar contains bb.edn w/ only :pods when bb.edn has :pods"
    (let [tmp-file (java.io.File/createTempFile "uber" ".jar")
          path (.getPath tmp-file)]
      (.deleteOnExit tmp-file)
      (let [config {:paths ["test-resources/babashka/uberjar/src"]
                    :deps '{local/deps {:local/root "test-resources/babashka/uberjar"}}
                    :pods (cond-> '{org.babashka/go-sqlite3 {:version "0.1.0"}}
                            (not (fs/windows?)) (assoc 'pod/test-pod {:path "test-resources/pod"
#_:here->                                                             :cache true}))}]
        (tu/with-config config
          (time (tu/bb nil "uberjar" path "-m" "my.main-pod"))
          (let [bb-edn-entry (get-entry tmp-file "bb.edn")
                bb-edn (-> path JarFile. (.getInputStream bb-edn-entry)
                           InputStreamReader. PushbackReader. edn/read)]
            (is (= #{:pods} (-> bb-edn keys set)))
            (is (= (:pods config) (:pods bb-edn))))
          (is (str/includes? (tu/bb nil "--jar" path) "3")))))))

borkdude15:04:20

although I set cache true

borkdude15:04:24

it's because of the local pod

borkdude15:04:59

I noticed it doesn't get anything from a cache

borkdude16:04:24

it's weird actually, creating the uberjar takes a long time... why, it doesn't actually have to load the pods at all I guess

borkdude16:04:38

oh maybe it does load those pods while it shouldn't have to, in the uberjar task

borkdude16:04:30

but anyway, it seems caching also didn't work, so two issues: ā€¢ cache for local path does not seem to do anything? ā€¢ loading of pods for namespaces can be prevented when creating an uberjar

borkdude16:04:56

Maybe the second one is not important, since we usually do downloading of deps etc too in the first usage of whatever. So it's mostly: why wasn't the cache working I guess

borkdude16:04:32

Or was that because the test uses a different dir each time? That could be

cap10morgan16:04:43

Yeah different dir is the likely culprit

cap10morgan16:04:22

I can work on optimizing that test a bit if you don't beat me to it :)

cap10morgan16:04:51

Otherwise local pod caching should be enabled when you don't opt out of it

borkdude16:04:58

Ah right. No, it's fine

cap10morgan19:04:52

And look into not loading pods when building an uberjar

borkdude19:04:15

Maybe not so important since when things are cached it won't take time

borkdude19:04:45

But might be good double checking if the caching is still ok in both registry and local pods

borkdude19:04:11

There are many things that do not need deps loading and pod caching, but we do it anyway, so I think it's best to leave it

šŸ‘ 1
borkdude16:04:00

@highpressurecarsalesm šŸ‘‹ I saw you were typing. Welcome back :)

Bob B16:04:12

I'm messing around with the :name metadata for #1223, and it seems like probably the most straightforward way to do it is to add :name in sci's new-var functions (there are a handful of accompanying tweaks in bb to use those functions more). My digging and testing on that has led me to a question that I'd like to confirm to make a better informed decision/proposal about an approach: I think that the intent is that a SciVar's sym field should be a namespace-qualified symbol - is that correct? That does seem to be the case in evaluator, but I think a lot of the copying ones use just a symbol name.

borkdude16:04:16

Good question...

borkdude16:04:03

I don't even know why we have sym to be honest. We could get the name from the metadata

borkdude16:04:54

unless we need to have a proper field for perf reasons

borkdude16:04:02

Maybe you could get an overview where we need the sym field and how it's typically used, in what functions

borkdude16:04:26

Then we could decide to either get rid of it and do it via the metadata or fix the field properly

Bob B16:04:20

from what I've seen, the actual sym field looks like it's primarily (maybe entirely?) used within the deftype

Bob B16:04:14

... and then other places call getName . I'll do some digging

Bob B20:04:34

Well, I've got all the sci tests passing with no references to the sym field (all the direct references are in the methods defined in the deftype), so it's doable entirely via metadata (notwithstanding potential performance concerns). My primary concern would be babashka, which, in a couple spots, creates SciVars without metadata (and quite possibly other consumers of Sci that might be calling the SciVar constructor directly instead of going through new-var.

borkdude20:04:20

Have you found out what potential performance implications might be? When is .getName called, typically? And what happens in the absence of the symbol?

borkdude20:04:47

One should not create SciVars directly, but always via the sci.core namespace, so I'll accept that breakage

borkdude20:04:12

(there's still places in bb where I call into .impl namespaces)

Bob B21:04:10

I only see one call to getName on a SciVar, and that's on an exception path to throw when someone tries to change a built-in var. All the uses of sym appear to be when getting a symbol/string to print a var name.

Bob B21:04:25

I'm convincing myself more and more that this is a good idea to go after:

before:
(->> (find-ns 'rewrite-clj.zip) ns-publics (take 5))
([child-sexprs #'child-sexprs] [tag #'tag] [insert-space-left #'insert-space-left] [postwalk #'postwalk] [whitespace? #'whitespace?])

after:
(->> (find-ns 'rewrite-clj.zip) ns-publics (take 5))
([child-sexprs #'rewrite-clj.zip/child-sexprs] [tag #'rewrite-clj.zip/tag] [insert-space-left #'rewrite-clj.zip/insert-space-left] [postwalk #'rewrite-clj.zip/postwalk] [whitespace? #'rewrite-clj.zip/whitespace?])
... with assoc'ing :name meta in new-var and removing namespaces from the names getting passed to it, the only outlier I see is *input*, and that should be pretty easy to address

borkdude19:04:47

Need to add a test for it, but works when manually tested.

borkdude20:04:01

One issue there is that multiple namespaces might load a pod namespace so I got to keep track of those

borkdude20:04:34

And probably move the load-pods near the top of the script

borkdude20:04:17

hmm this might be already solved since we dedupe code snippets!

borkdude21:04:32

@cap10morgan Merged the uberscript support now. Declarative pods in bb.edn v1 is done I think :)

šŸŽ‰ 1