Separating out the Policy Store, Policy Reasoner and API#61
Separating out the Policy Store, Policy Reasoner and API#61DanielVoogsgerd merged 82 commits intomainfrom
Conversation
| type Error = Infallible; | ||
|
|
||
| // fn visit_task(&mut self, task: &workflow::ElemTask) { | ||
| // // FIXME: Location is not currently sent as part of the workflow validation request, |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
| // let location = commit.location.clone().unwrap_or_else(|| String::from(ASSUMED_LOCATION)); | ||
| // self.read_sets.extend(repeat(location.clone()).zip(commit.input.iter().cloned())); | ||
|
|
||
| // // TODO: Maybe create a dedicated enum type for this e.g. NewDataset for datasets that will be |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
| // self.write_sets.push((location.clone(), Dataset { id: commit.data_name.clone() })); | ||
| // } | ||
|
|
||
| // // TODO: We do not really have a location for this one right now, we should figure out how to |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
|
Don't merge yet. Working on a final few fixes which you did but aren't completely taken over during the rebase. WIP until I fix this flippin' cannot-fetch-from-gitlab issue. It's really extremely weird that it's occurring here as well now. No clue what causes this behaviour. Did confirm it's Cargo somehow hanging on a git fetch, though. |
|
OK, should be mergable now! Fixed the gnarly git fetch error by avoiding the redirect in Gitlab. Seems like |
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Yay, finally passing all checks 🎉 |
DanielVoogsgerd
left a comment
There was a problem hiding this comment.
Pretty good, most of it is good stuff. Let's try to make it more idiomatic and maintainable.
| macro_rules! tuple_impls { | ||
| // Public interface | ||
| ($fi:tt $(, $i:tt)*) => { | ||
| // First, let's reverse the tuple | ||
| tuple_impls!(pair ($fi, $fi): $($i),* :); | ||
| }; | ||
|
|
||
| // Pair up the items | ||
| (pair ($fi:tt, $pi:tt): : $(($pri:tt, $ri:tt)),*) => { | ||
| tuple_impls!(reverse ($fi): $(($pri, $ri)),*:); | ||
| }; | ||
| (pair ($fi:tt, $pi:tt): $i1:tt $(, $i:tt)*: $(($pri:tt, $ri:tt)),*) => { | ||
| tuple_impls!(pair ($fi, $i1): $($i),*: $(($pri, $ri),)* ($pi, $i1)); | ||
| }; | ||
|
|
||
| // Reverse the items | ||
| (reverse ($fi:tt): : $(($pri:tt, $ri:tt)),*) => {}; | ||
| (reverse ($fi:tt): ($pi1:tt, $i1:tt) $(, ($pi:tt, $i:tt))* : $(($pri:tt, $ri:tt)),*) => { | ||
| tuple_impl!($fi, $i1, $(($pri, $ri),)* ($pi1, $i1)); | ||
| tuple_impls!(reverse ($fi): $(($pi, $i)),* : $(($pri, $ri),)* ($pi1, $i1)); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Is this you enjoying yourself or actually necessary? It is so error-prone and hard to read.
There was a problem hiding this comment.
Hahaha 😂 are they mutually exclusive?
No OK let me check what this actually is supposed to do xD
There was a problem hiding this comment.
Ah yes. I'd definitely say it's worth it's while. Remember that Rust has no way to abstract over tuples, so if you ever want to implement something for multiple tuples, you have to manually do the implementation for every tuple of size 0..n. This macro does all of those for you.
That said, this code is basically inspired by what I did over at Snack with the tuple-impls, which was a struggle at the time. However, as it happens, I was doing something like this again last night and accidentally ran into a MUCH SIMPLER way of doing this lmfao. For reference:
macro_rules! tuple_impl {
// Private interface
(__impl $($pi:literal),* : $fi:literal $(,$ni:literal)*) => {
// Implementation goes here. Use $($pi,)* $fi to get 0, 1, ..., n, where
// `n` is the size of the tuple - 1 (i.e., zero-indexed). You can also use
// the `paste` crate to generate identifiers.
// Recurse to implement size n + 1
tuple_impl!(__impl $($pi,)* $fi : $($ni),*);
};
(__impl $($pi:literal),* :) => {};
// Public interface
($fi:literal $(, $i:literal)* $(,)?) => {
// We do this to avoid the user having to do as little as possible thinking;
// we'll inject the `:` to separate between the "previous i's" and "future i's"
// manually.
tuple_impl!(__impl : $fi $(,$i)*);
};
}
// Implement like:
// (Implements for tuples of size 1, 2, 3, ..., 8)
tuple_impl!(0, 1, 2, 3, 4, 5, 6, 7);(yes that's the entire thing, two two macros are collapsed into one now)
| pub fn optimize(&mut self) { | ||
| let Self { start, .. } = self; | ||
| /// Optimizes the workflow graph by pruning empty branches. | ||
| struct DeadBranchPruner { |
There was a problem hiding this comment.
Ghmm, this gets me thinking: Should we optimize on the worker side or should the central node do the optimization and then force everybody to use that optimized workflow?
It feels a bit weird to optimize on here.
There was a problem hiding this comment.
That's a fair observation. I think you're right, in that in principle, the central node should do the optimization.
That said, the policy-reasoner version of a workflow is so much easier to optimize 😭 Plus, some of them (e.g., function inlining) done on the Brane one aren't just for fastness but also because that isn't supported in the checker version of a workflow.
Finally, note that eFLINT can be very slow, combinatorial-explosion-y. So there's incentive for workers to double-check that any workflows coming in are as small as possible. The question is, can they trust every workflow they get to be optimal.
Probably, the best way is: implement the optimizations as a library, then do it both at the central node and the worker node. If the central node does its job correctly, the worker node has little to do but scan the workflow once; if it doesn't, the worker can save time by investing some energy into optimization to relief (probably more) energy at the eFLINT step.
There was a problem hiding this comment.
Btw, the Brane version of a WIR is the b(r)ane of my existence. I wish I could do it all over again with what I learned, I think I can make it so much elegant, easier to work with and canonical with what I know from Rust now. Plus, we'll just ditch BraneScript harder than a coconut ~ we work with it because Brane started out that way, but really, it doesn't make a lot of sense. We're trying to express a graph, not a scripting language, so the whole compiler/workflow part of Brane has to bent over in a thousand different ways to make the script make an ounce of sense.
72dc21e to
82a25c9
Compare
This branch will contain the work on the `policy-reasoner` to turn the repository into a library instead of a binary. This is done to remove the Brane-specific parts of it, and put that into the Brane repo. As such, end binaries will be in the codebase of the respective project; the exception is the existing binaries (kind of), which are now examples. So far, moved the most important parts. Some work still needs to be done to re-introduce the appropriate logging, and fixing the POSIX reasoner. The rest -i.e., Brane parts and the store-part- need to be moved to independent repositories. The same applies to the eFLINT base policy (or that has to live in Brane land, idk).
Settled on having the paths come into a config instead of the ol' Brane way. Next up: extending logging, re-introducing contexts.
Also added a test example for the POSIX reasoner
82a25c9 to
eff880f
Compare
…ections This to avoid Brane WIR-like problems with long, linear workflows that cause the stack to overflow.
This because we decided to use the logger from different threads.
This is probably not the lowest we can go, but it is already a lot lower and fairly compatible with other minimal versions in the Brane framework
Also added some OG POSIX reasoner authors to the `Cargo.toml` file.
These functions were made async by hand, but can just use the async keyword
BTreeSet is in-order and unique, both properties we need. This way we don't have to allocate a vec afterwards.
And import the readme for documentation
Not really a problem now
f96e463 to
06f55a6
Compare
In particular the policy reaoner documentation reflected an implementation from a previous student assignment. It did not reflect the current work. This update loses quite a bit of information, but it should be more relevent to the users
048a6c2 to
ffc5100
Compare
|
Works on my machine :) (The M-mac mini, that is) |
This is the
policy-reasonercounterpart to BraneFramework/brane#270As discussed before (#42), the current co-dependency between the policy reasoner and the Brane repository is quite gnarly. This PR addresses this by completely refactoring the policy reasoner project into three parts:
As a side part of getting the whole project working again, implemented a fourth backend, the eFLINT Haskell interpreter. It is considered much more stable and complete than the previous "canonical" backend, the Go-implementation. It may be removed in a future release.
You can merge this PR first before you merge the counterpart at the Brane repo.
Fixes #39
Fixes #42