feat: Graph IR #6
Loading…
Reference in a new issue
No description provided.
Delete branch "(deleted):graph-ir"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Depends on #2. Merge that first.
Aims to introduce the graph IR in actual code.
WIP: Graph IRto WIP: feat: Graph IR71dddade6a
to2c147fd77b
Fwiw for some reason the
Conversation
tab (this one) says that the broken commit is still contained in this PR. However, theCommits
tab does not, which should be correct as I just force-pushed and removed the broken one.2c147fd77b
tof0e206a8eb
just fmt
3dc36dfb3ee1f5d11ab8
to2e6bd6890b
Also, since I have no idea if I even can review an image: do you know the licensing situation? Maybe use something that's CC-* for sure or that we've created instead?
@ -0,0 +9,4 @@
//! - [`Instruction`]s are referred to as their **byte [`Span`]s** in the source code,
//! so effectively where they are written in the source code.
//! (Or if coming from [`crate::semi_human::GraphIr`],
//! it's just a fictional number floating in space.)
Why a span then? Why not an id or something?
The span is the ID. It allows the executor to refer back to the source in errors or warnings.
i dont think a span (debug info imho) doubling as an id is a good idea
i think that should be seperate, so it doesn't get cursed to work with for other implementations that aren't text-based?
semi_human::GraphIr
atm, it's also easily possible to just usefrom
andto
as pseudo-IDs, which are justusize
s.The frontend also takes back the result, be it as a pointer to some gpu buffer or whatever else (that is something yet to be solved...) to be able to work with it, even if it just saves an image.
That would make fast GUIs possible, which is something that I'd consider important.
That's a hack. And if we do that now, it'll stay in the implementation that way forever, potentially leading to confusion in other implementations.
Sounds sensible, I'll convert the ID to be a pure
u64
then.@ -0,0 +26,4 @@
/// It does **not** contain what kind of instruction this is.
/// Refer to [`crate::instruction::Kind`] for this instead.
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
pub struct Instruction(pub(super) Span);
I think at least one more field would be good: metadata (or whatever name).
That field would have the use that the runtime always returns it, for debugging reasons (especially useful for a future client-server architecture)
To whom would the runtime "return" the metadata?
the implementor of the ir (the frontends), since for example the language implementation would just call the runtime and then as a response either get a result or if it fails errors (including metadata)
though especially in case of the language i think that it should check types etc to avoid errors (and a lot of checking, potentially on ir level that uses and generates metadata)
Not sure if I understand. When would instructions ever be given back to the frontend?
not instructions, the metadata field
which may contain spans, typed information etc to generate good errors.
else we'll just be yet another program with useless runtime errors, and that's not what I want to create.
Hm. I'm not sure if the metadata is worth polluting
GraphIr
with generics everywhere.Different suggestion then: How about being transparent about
id::Instruction
s, and allowing the frontend to keep secondary maps then?Do you mean returning the ID of the place it failed and potentially more information (depending on the errors, edges, other ids...)?
Yeah, exactly. The frontend could then just
GraphIr::resolve
if they want more information about the instruction that failed. Or use the ID to look up in their separate map built at source → graph IR conversion.I'd rather avoid the work on introducing metadata fields which
Instruction
would be generic over, sinceInstruction
s programmatically.Thanks for clarifying, makes sense! :3
@ -0,0 +68,4 @@
pub struct Socket {
pub belongs_to: Instruction,
pub idx: SocketIdx,
}
Isn't a u16 quite small in case of a huge graph? Also, how do named sockets work then?
The
SocketIdx
only refers to the sockets on one instruction. So one instruction could hold at most2^16 - 1 = 65535
inputs and outputs each at the moment. It is not a global ID,SocketIdx
are necessarily re-used across different instructions.Named sockets are a proposal at the moment, and not implemented yet. After the proposal would be accepted, the
SocketIdx
would instead be thrown into aString
(or an opaque wrapper type thereof).I'll clarify the u16 part on the docs of
SocketIdx
.Ahhh, I see.
I do think that there should only be at most one value because that might make some stuff a lot more elegant, but that's also solvable via syntactic sugar probably (and this is also the wrong place to discuss this)
I think this discussion doesn't belong in a code review, probably in #4.
Nevermind, I am just rambling out of my unhealthy functional programming addiction (and I really like the way nix does it)
@ -0,0 +60,4 @@
Self::Write(_) => (1, 0),
Self::Math(_) | Self::Blend(_) => (2, 1),
Self::Noise(_) => {
todo!("how many arguments does noise take? how many outputs does it have?")
noise is an instruction type. so are the others, we'll need more complex code to determine the correct numbers here in the future...
@ -0,0 +68,4 @@
pub struct Instruction(Span);
impl Instruction {
/// Where this instruction is written down.
do you mean like "written down in code"? or am i misunderstanding?
This whole project screams for tests lol
No, you're correct. I'll update the docs to be more specific.
@ -0,0 +120,4 @@
/// Theoretically this could be fixed easily at the expense of some memory
/// by just incrementing and storing some global counter,
/// however, at the moment there's no compelling reason
/// to actually have multiple [`GraphIr`]s at one point in time.
A reason why that might happen is reusable blocks aka user defined functions?
Those would be still in the same
GraphIr
.How do you intend to implement custom function calls then? Rough outline because currently I don't think I understand
goto
s kinda or...?Edges to a different part of the graph. One could think of edges as being kind of
goto
s.that doesn't solve namespacing for said functions.
and imports (which will be necessary in the future anyway), so multiple
GraphIr
s will be necessary to load "libraries", however those may look. So i'd prefer a function to not be just connected via edges, especially since that'd be an absolute mess in a gui.I'd argue the concept of "namespaces" or even function names doesn't belong into an executor or into the graph IR, but rather into the frontend.
Why are multiple
GraphIr
s necessary to load libraries? Those can be added to the same, globalGraphIr
. This program also features 2 completely disjoint evaluation paths that don't correlate, but can still be thrown into the sameGraphIr
:Namespacing can also just be an ID.
But I take back that library loading may be in the IR, since that makes distribution as binaries possible.
I misunderstood how your implementation works and how you (and even myself somehow) understood it.
I take back my concerns.
@ -0,0 +159,4 @@
///
/// # Panics
///
/// Panics if there are any cycles in the IR, as it needs to be a DAG.
Should later probably more of a graceful exit...
@ -0,0 +1,90 @@
//! The midterm solution for source representation, until we've got a nice source frontent.
typo: frontend
@ -0,0 +8,4 @@
2: Write((
target: File("testfiles/inverted.png"),
format: Png,
)),
a map of numbers to instructions could also just be a vector?
Technically, yes. However, since the edges directly below refer to the instruction IDs directly, I believe listing the IDs of the instructions explicitly is worth it, as otherwise one would keep needing to count from the beginning of the list to find out the index to write into an edge.
If i understand your implementation correctly, would it theoretically be possible to also allow named instructions then for usability reasons? even if they just get abstracted away (and the name then put into the metadata which i suggested for example)
Yes, that would be possible. The
semi_human::GraphIr
could use string interning for generating IDs then.I'm not sure if or how per-instruction metadata would be useful here, since some kind of storage that is keyed by instruction name in order to decide if to either
Idk, I just think that that might be useful for hand-written IR (which, don't get me wrong, is not the indended use case obviously. But that might make it a lot easier to debug more complex stuff for us in the future)
Yeah, then I don't see how metadata is useful for the hand-written IR.
The named instruction thing is something worth to discuss though, imo. Tbh, a lot of the time I'd have difficulties finding names for each instruction, and either resort to just again using numbers or just following the pattern of
<instr>-<arg>
, both of which seem counterproductive to me.For example, what would you name each instruction in this case (taken from the design doc)?
In that case, I wouldn't name them anyway. But I think that might be easier in some cases for some people. However, I take the idea back since it's tbh useless for debugging since that's not the use case (you're not supposed to use iOwO to debug it...) and that's not the target audience.
No, I do not know the license of the juan meme image. I'd be happy for any alternative suggestions you might have though!
These random rails for now? I know the license situation because well... I made it myself. Probably isn't optimal either, though.
What license do you put it under, then?
CC-BY-SA.
Not that I care much about it, but yeah.
Once the project progresses, I'll maybe bother rendering it with a lot more data (normals, cryptomatte, whatever) for debugging/example purposes. Or we can just make dedicated example images then!
CC-BY-SA 4.0?
@schrottkatze The latest commit included the new licenses, both of the image and the code as a whole. Are you okay with those licenses?
AGPL is lovely!
Maybe also add a pointer to the node each time? Or do you think that'd have a significant performance impact or something?
Is that explicit agreement that you're okay with both the CC BY-SA 4.0 and the AGPL?
3b3a3c978e
to52f8bda516
Yes. The image should be under CC-BY-SA 4.0 and my code under the AGPL.
The "node" is the instruction, and my idea was that a socketidx might contain a reference to the instruction it belongs to so it's easy to reference to it and hard to fuck up
That is already done in
Socket
.SocketIdx
is only really useful in context of aSocket
. It only exists as a newtype to avoid accidental modification of the containedusize
(which we'll need to change for named sockets anyway).ahh I see!
efb83c3796
toc36a17d313
c36a17d313
to98c9399ee4
WIP: feat: Graph IRto feat: Graph IRDone. The eval impl is hacky, but it works for a quick demo of how to use the graph IR.
One general note (mostly for later, which I'll work on in the next few days): The runtime should be able to mix and match executors, so one executor may not fully support all instructions but for that offer more performance (because some shit might be harder to implement as compute shaders) so switching on the fly is possible.
Also useful if you want a debug-only instruction that's new and experimental but the rest of the graph is cpu/vulkan compatible already, so you can have performance and new instructions.
@ -44,3 +48,3 @@
Some("ron") => Ok(serde_json::from_str(&fs::read_to_string(p)?)?),
Some("json") => Ok(ron::from_str(&fs::read_to_string(p)?)?),
e => Err(ConfigError::UnknownExtension(e.map(|v| v.to_owned()))),
e => Err(Config::UnknownExtension(e.map(str::to_string))),
I generally prefer using
to_owned
, since it's more clear that you're turning an unowned string (&str
) into an ownedString
.@ -17,0 +21,4 @@
fs::read_to_string(cfg.source).expect("can't find source file lol handle me better please");
let ir =
ir::from_ron(&source).expect("aww failed to parse source to graph ir handle me better");
i mean yes, maybe slap one (or two)
TODO
comments there?@ -0,0 +19,4 @@
impl crate::Evaluator for Evaluator {
fn feed(&mut self, ir: GraphIr) {
// TODO: should add instead of replace, see note in Evaluator trait above this method
meow? is this referring to the note from line 14-16 in the struct?
No. It is referring to the
Evaluator
trait atcrates/eval/src/lib.rs:9
and to the comment on line 12f.Actually, if it causes this much confusion, I'll just remove the comment here to make the other one the single source of truth.
@ -0,0 +64,4 @@
let output = match instr.kind {
Kind::Read(details) => Some(Variant::Image(instr::read::read(details))),
Kind::Write(details) => {
#[allow(irrefutable_let_patterns)] // will necessarily change
wdym?
but yes, we should add more output methods.
There will necessarily be more variants added to
Variant
later on, so the lint is suppressed since theif let
will be necessary then. The comment refers to this circumstance, not to the output methods (whatever you may mean with that).Ahh okay! I meant more sinks/consumers
and potentially for write to also be able to write csvs etc
@ -0,0 +10,4 @@
/// Take some [`GraphIr`] which will then be processed later.
/// May be called multiple times, in which the [`GraphIr`]s should add up.
// TODO: atm they definitely don't add up -- add some functionality to GraphIr to
// make it combine two graphs into one
Is this the note that was referred to in the other file? Or the one inside the struct?
It is referred to by
crates/eval/src/kind/debug/mod.rs:22
.@ -0,0 +5,4 @@
/// The name is taken from [Godot's `Variant` type],
/// which is very similar to this one.
///
/// [Godot's `Variant` type]: https://docs.godotengine.org/en/stable/classes/class_variant.html
I'm going to rework this to be much more statically typed, but for now this should be fine.
Probably also the values/data should be able to handle more complex use cases like building a graphics pipeline and then running it all at once for performance reasons, but that's all future brabbling for... well the next few days.
Sure, however you plan on implementing that, since the evaluation cache of the evaluator needs to store something.
I do not see how this is related to
Variant
at all.Variant is the internal way to store data, yes?
And that must be able to store more data then an image buffer or a few numbers or whatever for more complex evaluations.
Especially to be able to correctly mix evaluators...
Probably gonna have to analyze and pre-build the pipelines etc, but that's out of scope of this conversation.
@ -0,0 +47,4 @@
/// On an **instruction**, returns outgoing data to be fed to [`Input`]s.
///
/// In contrast to [`Input`]s, [`Output`]s may be used or unused.
note to keep in mind for later, maybe this should also have the option to have default values? probably rather relevant for frontends...
Could you possibly give an example where a default value on an instruction would be desirable?
you dont always wanna give all parameters to a noise generator for example
kinda want a system for named args
but i have no idea how to implement that in the language lol
Named args are implied by #4.
@ -0,0 +15,4 @@
/// Look at [`semi_human::GraphIr`] and the test files in the repo at `testfiles/`
/// to see what the RON should look like.
/// No, we don't want you to write out [`GraphIr`] in full by hand.
/// That's something for the machines to do.
potentially clarify that that's just for us, the devs, debugging?
That's why it is in a doc comment.
hm yes i am big brain sorrry ^^'
@ -0,0 +1,86 @@
//! The midterm solution for source representation, until we've got a nice source frontend.
And for development probably later on as well, though the runtime/server/whatever should be able to provide enough info to do that comfortably as well.
I plan to get rid of this as soon as we have a meaningful
source
→ir
pipeline, actually.I think it makes sense for debugging purposes. Or do you plan to add a way to be able to output the IR to a more readable form of IR (that has a similar syntax, similar to the human readable forms of rusts HIR/MIR?)
I want
ir::GraphIr
to be the single source of truth regarding the IR, and not a programmatic in-between that is kinda-for-human-interaction-but-not-really that breaks type safety.For debugging purposes of the IR, I want to add a DOT and typst output later. I'll open issues about those.
@ -8,3 +8,3 @@
)
= Evaluation stages
= Processing stages
Ok seriously we probably need to have a meeting and talk about the terminology ^^' this is really becoming a headache and a mess
Anytime.
Assumed somewhere behind this statement is a request for a action: Should I add a short overview of terms in the design doc?
I'll just do that, actually.
Did that in
63d7993940
on page 2 of the design doc. Could you possibly look over it and see if it satisfies your desires?They are distinct since if you write a function on whatever frontend you use, that'll be compiled down to IR and split up. An instruction will however generally not be split up (unless... looks at future optimizer... but that's not relevant yet.). So Instructions are a subset of functions.
And I'd say that we specify a Streamer as a function that generates output without any input, and add a "sink" as a function that just takes in inputs and is effectively the end of a given pipeline.
Why? The current nonexclusive wording makes talking about pipelines easier. We can introduce new terms to mark those absolute starts and ends if deemed useful, but I can't think of a case where one would want to use them.
The only instruction that could be such a start of a pipeline will be
Const
anyway.Also,
has one major pipeline (everything), but actually 2 more pipelines:
open lol.png | invert
andinvert | save idk.png
. Do you want to remove those two and make a new term "subpipeline"? If so, why?Also, I propose you add Nix and direnv to the
CONTRIBUTING.md
s tech stack list.Potentially with a note how to use the flake and envrc?
@ -1,2 +5,4 @@
cargo nextest run
# Compile all documentation as in proposals and design documents, placing them under `docs/compiled`.
docs:
Lets add an alias for the
docs
recipe that's justdoc
, since I keep typing that out of habit.Makes it simpler for everyone to use whatever they want.
I've decided I do not want to contribute to iOwO anymore, due to lack of general cooperativity and @schrottkatze not willing to discuss things such as using vulkano that are, apparently, set in stone since the project's beginning.
Feel free to do whatever with this PR, the license as defined in the
LICENSE
under this PR persists. You can still fetch it locally throughEDIT: After conversing with Schrottkatze locally, I decided to continue contributing under some conditions.
Pull request closed