Fork me on GitHub
#babashka-sci-dev
<
2022-03-19
>
borkdude09:03:28

@wilkerlucio I'll take the discussion here about meander. ๐Ÿงต

borkdude09:03:56

I've ran the tests:

Ran 155 tests containing 316 assertions.
2 failures, 11 errors.
{:test 155, :pass 303, :fail 2, :error 11, :type :summary}
So there are still a few glitches to iron out, hopefully we can make them pass all.

borkdude13:03:57

There seems to be an issue with gather

user=> (babashka.main/main "-cp" "src" "-e" "(require '[meander.epsilon :as r] :reload '[meander.match.epsilon] :reload) (r/match [1 2 3 4 5 6] (r/gather (r/pred any? !xs) ...) !xs)")
:count-pattern ...
:meander/match :syntax-phase
false :nat-int
true :symbol ...
----- Error --------------------------------------------------------------------
Type:     clojure.lang.ExceptionInfo
Message:  non exhaustive pattern match
user=> (babashka.main/main "-cp" "src" "-e" "(require '[meander.epsilon :as r] :reload '[meander.match.epsilon] :reload) (r/match [1 2 3 4 5 6] (r/pred any? !xs) !xs)")
[[1 2 3 4 5 6]]
0

cap10morgan15:03:27

@borkdude OK think I addressed everything in the two PRs. Let me know if you see anything else.

cap10morgan15:03:32

oops, looks like I still have an issue to address with :local/root pods

cap10morgan15:03:31

or... perhaps not. anyway, testing that out now. I'll keep you posted. ๐Ÿ™‚

borkdude15:03:47

looks pretty good so far!

borkdude15:03:22

apart from the minor remarks, maybe we'll have to do a final bike-shed over the naming.

:local/root
is derived from deps.edn, and makes sense since it's the root directory of a dependency, but pods aren't directories. I think just using :path makes more sense, like the name that the pods lib already uses for it. We could make everything namespaced:
:pod/version ... :pod/path ...
but not sure if that has any benefits since we're already dealing with pods. However, we do use that in the manifests:
:pod/name        huahaiy/datalevin
 :pod/description "Datalevin Database"
 :pod/version     "0.6.1"
 :pod/license     "EPL"

borkdude15:03:54

but there are also other things like :os/name etc

cap10morgan15:03:30

I think :version and :path are good. I think I'd want to leave the namespacing for something it might differentiate down the road. Like a different kind of version, for example. But like you said, it's already in a pods context here.

borkdude15:03:35

multimethod can only dispatch on namespaced keywords right?

borkdude15:03:04

not that that matters here (yet)

cap10morgan15:03:07

hmm... not sure. I didn't think so

cap10morgan16:03:03

Hmm... getting tripped up by one thing: in b.pods.impl/resolve-pod it checks if pod-spec is a qualified symbol and calls resolver/resolve on it if so. but for my local pod implementation, the pod-spec is still a qualified symbol, it just has a :path opt instead of a :version one. OK to change that check to (when version ...) ?

borkdude16:03:34

@cap10morgan One more idea. As there is a discrepancy between local pods (which is really a dev thing maybe), we could do this: Support a :namespaces [...] option in bb.edn, as a requirement for being able to use local pods.

{:pods {foo/bar {:path "../dude" :namespaces [foo.dude foo.dude2]}}}
Then there is no need to call the local pod to inspect the namespaces.

borkdude16:03:14

and this :namespaces option overrides whatever is in the metadata, so you can tweak it further, e.g. disable it for a certain namespace for whatever reason

cap10morgan16:03:26

where is the discrepancy for local pods? I personally would not want to have to manually maintain a separate namespaces list in there while developing a pod. especially when the same code is already 99% capable of asking the local pod for this info. I just need to iron out this resolver issue.

borkdude16:03:45

the discrepancy is "local describe cannot be cached"

cap10morgan16:03:13

I mean, it could be but then cache invalidation gets tricky so I wanted to avoid ๐Ÿ™‚

borkdude16:03:37

we could support bypassing the metadata with bb --force

borkdude16:03:49

which is already an option to bypass the clj .cpcache file

borkdude16:03:58

for development

borkdude16:03:08

so it will be cached by default, unless --force is used

cap10morgan16:03:20

that seems reasonable

cap10morgan16:03:21

I think I still have a slight preference for just not caching it in a dev scenario. but local pods might not just be for dev, I guess. I was using one in prod before I got it into the registry. custom registries is probably the better answer there, though.

borkdude16:03:03

ok so not caching local pods then?

cap10morgan16:03:20

that feels simplest and lowest support burden (for you, honestly) to me

borkdude16:03:21

should we even support local pods in bb.edn? I don't even know how we arrived at that

cap10morgan16:03:36

it would be really nice for pod development

borkdude16:03:56

but you can use a local registry for that

cap10morgan16:03:58

yeah, but with a lot of the same downsides (immutable versions or you need to bust the cache, etc.) versus just reloading the in-development code as it changes

borkdude16:03:51

What about an option :path ... :cache false

borkdude16:03:31

I could see a scenario where people in the cloud have their pods in a specific place and would still like to cache the namespaces for better warm startup times

cap10morgan16:03:22

maybe :cache false is the default but you can turn it back on for local pods with :path ... :cache true?

borkdude16:03:44

the default should not be the development setting

cap10morgan16:03:31

yeah, makes sense. the counter-argument for me is that I like defaults for non-exasperating correctness (i.e. why isn't my new namespace getting picked up?) and knobs for perf tuning

cap10morgan16:03:50

but happy to defer to you here ๐Ÿ™‚

borkdude16:03:57

I think the default should be the performant option

๐Ÿ‘ 1
borkdude16:03:25

as in: people are going to try this on the cloud and we want them to succeed before they give up without even looking at that option ;)

borkdude16:03:54

back to your original issue, what was that again?

borkdude16:03:01

oh yes, that seems ok

borkdude16:03:21

maybe (and qualified-symbol version)?

๐Ÿ‘ 1
borkdude16:03:42

I think we should also have some tests, but if the pod tests are still passing, we could just test it from the bb side

borkdude16:03:17

So we have:

:pods {fq/sym {:path "/foo" :version "0.1.0" :cache false}}

cap10morgan16:03:33

yes definitely

borkdude16:03:53

Let me know if the pod side is ready for merge, then I'll do that first

cap10morgan16:03:58

you wouldn't have version w/ path. one or the other.

cap10morgan16:03:14

will do (still working on local pods issue)

borkdude16:03:31

yes, agreed, they are mutually exclusive.

cap10morgan16:03:42

ah, you were just listing the options. gotcha

borkdude16:03:05

This is going to be sweet :)

cap10morgan16:03:44

yeah I'm excited about it ๐Ÿ™‚

cap10morgan16:03:56

think we should allow :cache false for registry pods too?

borkdude16:03:17

yeah, why not

๐Ÿ‘ 1
cap10morgan17:03:23

do you think this will warrant a version bump to 0.8.0? started updating the pods README to document bb.edn pods declarations and wanted to say "As of babashka [version] you can..." I can just use a placeholder there for now if that's better.

borkdude17:03:10

Yeah I'm ok with 0.8.0

๐Ÿ‘ 1
cap10morgan17:03:55

where should we store the cache file for local pods? same dir as the pod binary?

cap10morgan17:03:11

or somewhere in ~/.babashka/pods ?

borkdude17:03:19

I think the latter would be nicer

borkdude17:03:39

maybe we can make a sha512 of the bytes of the pod for the filename

borkdude17:03:23

or just replace the file delimiter with a different char

borkdude17:03:53

like ~/.babashka/pods/.cache/Users$borkdude$dev$pod-foobar.cache

cap10morgan17:03:58

the hash of the pod binary itself is intriguing... solves cache invalidation and re-uses the cache even if you move it around. :thinking_face:

borkdude17:03:30

also no problems with relative path differences

cap10morgan17:03:20

do we even need the :cache param w/ that?

borkdude17:03:39

probably not :)

cap10morgan17:03:49

I like it! good idea!

borkdude17:03:58

well maybe it's still a good idea to be able to disable the cache in case we get our hashing wrong

borkdude18:03:35

but we can try without first

borkdude18:03:38

and solve that problem later

borkdude18:03:46

I've got some hashing code in this library: https://github.com/nextjournal/dejavu

borkdude18:03:06

which in turn uses mvxcvi/alphabase

borkdude18:03:11

we can probably inline that code

cap10morgan18:03:50

do you want it in base58 or is base64 ok? (assuming babashka has java.util.Base64 available)

borkdude18:03:36

don't remember why we chose base58, maybe because it plays better with browser urls

borkdude18:03:52

base64 is ok with me though

borkdude18:03:02

provided that file delimiters aren't part of base64

cap10morgan18:03:12

yeah, I'll double check

borkdude18:03:31

so : , / and \\ should not be part of that set

cap10morgan18:03:13

ok the url encoder can produce safe base64

borkdude19:03:48

safe as in, safe file path?

borkdude19:03:52

on Windows and linux?

borkdude19:03:54

then it sounds good to me

borkdude19:03:32

hmm, 63 is /

borkdude19:03:05

ah right, URLEncoder should do the trick