allora-network / allora-chain

Node software to run the Allora Network
https://www.allora.network/
Apache License 2.0
78 stars 72 forks source link

resolve exhaustruct on synth while preserving builder #621

Closed kpeluso closed 1 month ago

kpeluso commented 1 month ago

Purpose of Changes and their Description

This is my alternative to https://github.com/allora-network/allora-chain/pull/538

I have absolutely been burned before for having too many arg params. I've fixed many bugs directly resulting from this. The pattern employed there is really not great in my opinion.

In this PR I ready synth for exhaustruct while preserving the patterns.

Link(s) to Ticket(s) or Issue(s) resolved by this PR

Are these changes tested and documented?

Still Left Todo

relyt29 commented 1 month ago

This proposal is effectively no change on inference synthesis, and keeps all of the parts of inference synthesis that are written completely differently than any other part of the codebase (nowhere else do we use this factory builder object oriented pattern, nowhere else has a palette).

If I look at a function today in the inference synthesis package I have no idea what specific parts of the synth palette it operates on from looking at the function header because the synth palette and network inference builder structs hide the actual elements used in the function.

This, paired with lack of exhaust struct, has caused bugs that affect the accuracy of inferences. This also has affected performance due to clones and passing of unnecessary values to every function rather than just what the function needs

kpeluso commented 1 month ago

@relyt29

spooktheducks commented 1 month ago
  • The thing with the "cloning vs passing it in" is that when we're passing in by value, as in your PR, we have to realloc the memory for each new scope it's passed to anyway, which is the same complexity as cloning, as we do here. So I'm actually skeptical of the optimization you found, and confident that if you did find one, we could implement it under this framework all the same.

Ensuring data immutability in Go is pretty simple -- here are the rules: 1) Go is C, so if you don't understand C, the stack, and the heap, you're fucked 2) If your struct doesn't contain pointers, maps, slices, or channels, everything is automatically cloned when passed through function boundaries or assigned to a new variable (pointers, maps, slices, and channels are all pointers to structs under the hood, fwiw) 3) You can't clone a channel. That's completely insane and would create ruptures in spacetime 4) One can clone slices and maps with the respective stdlib slices and maps packages, or just write for loops if one has the muscle mass to support it 5) If you have pointers to structs, make sure you deference them when cloning, like this:

childStructToClone := *x.foo
clonedStruct.foo = &childStructToClone

Boom, it's cloned. Unless it has maps, slices, pointers or channels. In which case you'll need a .Clone method on it that does stuff like slices.Clone(x.foo) etc.

Therefore, if you're writing a .Clone() method: 1) use a non-pointer receiver so all of the fields are cloned automatically. Saves you some lines 2) any of the fields that are pointers, maps, or slices -- clone those manually with the stdlib 3) only clone channels at CERN

kpeluso commented 1 month ago

Closing in favor of https://github.com/allora-network/allora-chain/pull/538