Read model type from R (attempt number 2) [PLT-1174] #5857
Conversation
…read-model-type-from-R-2
|
/benchmark validation |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'nofib' on '902cfa3ac' (base) and 'd77e0f56c' (PR) Results table
|
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'validation' on '902cfa3ac' (base) and 'd77e0f56c' (PR) Results table
|
|
/benchmark validation |
|
/benchmark nofib |
kwxm
changed the title
kwxm
changed the title
|
I was mystified by the apparent speedup in |
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'validation' on '902cfa3ac' (base) and 'd77e0f56c' (PR) Results table
|
|
Click here to check the status of your benchmark. |
|
Comparing benchmark results of 'nofib' on '902cfa3ac' (base) and 'd77e0f56c' (PR) Results table
|
There was a problem hiding this comment.
A lot of that is above my head, but here are some comments.
Generally, I think we're moving in the opposite direction of what I consider to be the right approach: making static info (shapes of costing functions) into static code inside of builtins where it's more convenient to review, maintain etc and where it would be faster too.
Is it in theory possible to supply R code with the shapes of costing functions and parse it there as opposed to doing it the opposite way?
| -- FIXME: we could use ModelConstantOrOneArgument for | ||
| -- ModelTwoArgumentsSubtractedSizes instead, but that would change the order of | ||
| -- the cost model parameters since the minimum value would come first instead of | ||
| -- last. |
There was a problem hiding this comment.
Yeah, instead of all those ModelAddedSizes, ModelSubtractedSizes etc we should've just introduced "anonymous" SlopeAndIntercept, SlopeInterceptAndConstant etc. Or even just completely remove those model types? Does anything relies on them existing as opposed to being inlined in ModelOneArgument, ModelTwoArguments etc?
There was a problem hiding this comment.
Does anything relies on them existing?
I think not, and I'd like to generalise all of this stuff, but as it says that would change the order of the cost model parameters and we don't want to do that at the moment. We have to worry about backwards compatibility as well. In this PR I'm trying to improve things as much as I can without breaking anything else. Once we have more configurability wrt PVs and so on we can start to make some other changes.
| -> R s (CostingFun model) | ||
| getParams readCF param = do | ||
| let memModel = getId $ param builtinMemoryModels | ||
| cpuModel <- readCF $ getConst $ param cpuModels |
There was a problem hiding this comment.
So we hardcode memory models, but not CPU models, if I'm reading it right? Why?
There was a problem hiding this comment.
That's the way it's always been, and that's been OK because memory models don't change too much. Also we're not doing any analysis to infer the memory models, just making them up based on what we know about the implemenations of the builtins. What we could do is have a separate (versioned) JSON file for the memory models and read them from there: you'd have to write the JSON by hand though, which might be a bit fragile. Maybe we could use some other format (which is essentially what we have in the Haskell now anyway). Actually maybe it's better just to keep them in buikltinCostModel.json anyway since if we eventually have mulitple versions of that it'd be easier to manage things if the memory models are in the same place as the CPU models.
There was a problem hiding this comment.
I was fixing something else in the Agda code and trimmed lots of trailing whitespace while I was at it.
I think not. The R code does quite a lot of other stuff, like selecting subsets of the data to fit a model to. We'd have to tell it all that as well and then we're moving towards a full-scale DSL for interacting with R. I think we'd end up translating much of the R code into more complicated Haskell. |
Perhaps I'm misunderstanding it, but I think all I need is for R to take a string or whatever and parse it, so that this: doesn't have I'm not at all confident that would work on the Haskell side, so I'm asking if it can work on the R side, 'cause if not, then I should just give up on that dream (or consider returning the kind of double-hardcoding that you just removed, which you probably wouldn't approve of haha). So basically the question is: do you think it's not possible for the Haskell code to know that |
That seems a bit tricky. We could probably do it for constant models, but the problem is that many of the models are quite precisely tailored to a particular builtin: you'd end up with things like Maybe we could make the type nested or something, but then we're kind of back to the DSL. I'm also keen to keep the use of The way I was thinking about having models that depend on the PV was to create a JSON file with the correct cost model types and coefficients for that PV and then commit it and leave it unchanged forever. If at a later date we decided to change the shape of a costing function then we'd rewrite the R code to include new modelling a modelling code and probably discard the old code: we wouldn;t need it any more because we would never need to regenerate the JSON files for previous PVs: they're fixed forever and contain all of the information you need recreate the costing function in Haskell. Maybe there's something bad about that plan that hasn't occurred to me though. |
It's moderately well-maintained, since it's a Tweag project. e.g. it has had commits in the last week. There was a period where it hadn't been updated for a while, though. |
kwxm
changed the title
|
Can I merge this? The important thing is that increases the flexibility of the Haskell models. It also reworks the R/Haskell interface, but that's only used during cost model generation; it removes some hard-coded numbers that have been annoying me for a while. We can revisit that part later if need be. |
|
Sorry, I haven't got around to it, but if you and Roman are happy it's going in the right direction then go for it! |
|
This is a continuation of #5841. It
These changes will lead to some small differences in the numbers in the JSON file, but I haven't updated it yet. It's probably better to wait for a full cost model update to do that. The one exception is the cost model for
verifyEd25519Signature, which was based on the wrong variable. I've taken the opportunity to correct that. The function is very fast anyway, so the differences weren't very significant: the correction has actually made the benchmarks cheaper. [This change has now been reverted until some other pending PRs which improve configurability have been merged.].I have some more improvements in mind for the core costing function code, but those will probably change the order of the cost model parameters so I'll leave it for a later PR.