feat: Graph IR #6

Closed
multisamplednight wants to merge 0 commits from (deleted):graph-ir into main
Collaborator

Depends on #2. Merge that first.

Aims to introduce the graph IR in actual code.

Depends on #2. Merge that first. Aims to introduce the graph IR in actual code.
multisamplednight added 25 commits 2024-01-11 10:56:14 +00:00
multisamplednight added 1 commit 2024-01-11 11:04:03 +00:00
multisamplednight added 3 commits 2024-01-11 13:03:38 +00:00
multisamplednight added 2 commits 2024-01-12 16:24:32 +00:00
multisamplednight changed title from WIP: Graph IR to WIP: feat: Graph IR 2024-01-18 18:02:12 +00:00
multisamplednight added 1 commit 2024-01-18 18:31:11 +00:00
multisamplednight force-pushed graph-ir from 71dddade6a to 2c147fd77b 2024-01-18 19:12:39 +00:00 Compare
Author
Collaborator

Fwiw for some reason the Conversation tab (this one) says that the broken commit is still contained in this PR. However, the Commits tab does not, which should be correct as I just force-pushed and removed the broken one.

Fwiw for some reason the `Conversation` tab (this one) says that the broken commit is still contained in this PR. However, the `Commits` tab does not, which should be correct as I just force-pushed and removed the broken one.
multisamplednight force-pushed graph-ir from 2c147fd77b to f0e206a8eb 2024-01-18 19:20:48 +00:00 Compare
multisamplednight added 3 commits 2024-01-18 20:40:01 +00:00
multisamplednight added 6 commits 2024-01-19 01:11:04 +00:00
multisamplednight added 1 commit 2024-01-19 02:00:18 +00:00
multisamplednight force-pushed graph-ir from e1f5d11ab8 to 2e6bd6890b 2024-01-19 02:11:56 +00:00 Compare
schrottkatze approved these changes 2024-01-19 08:38:29 +00:00
schrottkatze left a comment
Owner

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?

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.)
Owner

Why a span then? Why not an id or something?

Why a span then? Why not an id or something?
Author
Collaborator

The span is the ID. It allows the executor to refer back to the source in errors or warnings.

The span _is_ the ID. It allows the executor to refer back to the source in errors or warnings.
Owner

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?

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?
Author
Collaborator

i dont think a span (debug info imho) doubling as an id is a good idea

  • Why not?

so it doesn't get cursed to work with for other implementations that aren't text-based?

  • As it's done for the semi_human::GraphIr atm, it's also easily possible to just use from and to as pseudo-IDs, which are just usizes.
> i dont think a span (debug info imho) doubling as an id is a good idea - Why not? > so it doesn't get cursed to work with for other implementations that aren't text-based? - As it's done for the `semi_human::GraphIr` atm, it's also easily possible to just use `from` and `to` as pseudo-IDs, which are just `usize`s.
Author
Collaborator
  • How would you want to report warnings/errors back from the executor then? Do you want the frontend to be responsible for keeping track of a potential ID → span mapping, even though after the Source → Graph IR step, the frontend's job is usually done?
- How would you want to report warnings/errors back from the executor then? Do you want the frontend to be responsible for keeping track of a potential ID → span mapping, even though after the Source → Graph IR step, the frontend's job is usually done?
Owner

How would you want to report warnings/errors back from the executor then? Do you want the frontend to be responsible for keeping track of a potential ID → span mapping, even though after the Source → Graph IR step, the frontend's job is usually done?

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.

Why not?
It makes things more complex, forcing it into one use-case and mixing up information. Spans should be in the metadata.

As it's done for the semi_human::GraphIr atm, it's also easily possible to just use from and to as pseudo-IDs, which are just usizes.

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.

> How would you want to report warnings/errors back from the executor then? Do you want the frontend to be responsible for keeping track of a potential ID → span mapping, even though after the Source → Graph IR step, the frontend's job is usually done? 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. > Why not? It makes things more complex, forcing it into one use-case and mixing up information. Spans should be in the metadata. > As it's done for the semi_human::GraphIr atm, it's also easily possible to just use from and to as pseudo-IDs, which are just usizes. 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.
Author
Collaborator

Sounds sensible, I'll convert the ID to be a pure u64 then.

Sounds sensible, I'll convert the ID to be a pure `u64` then.
multisamplednight marked this conversation as resolved
@ -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);
Owner

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)

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)
Author
Collaborator

To whom would the runtime "return" the metadata?

To whom would the runtime "return" the metadata?
Owner

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)

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)
Author
Collaborator

Not sure if I understand. When would instructions ever be given back to the frontend?

Not sure if I understand. When would instructions ever be given back to the frontend?
Owner

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.

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.
Author
Collaborator

Hm. I'm not sure if the metadata is worth polluting GraphIr with generics everywhere.

Different suggestion then: How about being transparent about id::Instructions, and allowing the frontend to keep secondary maps then?

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?
Owner

Do you mean returning the ID of the place it failed and potentially more information (depending on the errors, edges, other ids...)?

Do you mean returning the ID of the place it failed and potentially more information (depending on the errors, edges, other ids...)?
Author
Collaborator

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.

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.
Author
Collaborator

I'd rather avoid the work on introducing metadata fields which Instruction would be generic over, since

  • they introduce quite some noise,
  • would require a lot of bounds and
  • introduce significant maintenance work once we start constructing Instructions programmatically.
I'd rather avoid the work on introducing metadata fields which `Instruction` would be generic over, since - they introduce quite some noise, - would require a lot of bounds and - introduce significant maintenance work once we start constructing `Instruction`s programmatically.
Owner

Thanks for clarifying, makes sense! :3

Thanks for clarifying, makes sense! :3
schrottkatze marked this conversation as resolved
@ -0,0 +68,4 @@
pub struct Socket {
pub belongs_to: Instruction,
pub idx: SocketIdx,
}
Owner

Isn't a u16 quite small in case of a huge graph? Also, how do named sockets work then?

Isn't a u16 quite small in case of a huge graph? Also, how do named sockets work then?
Author
Collaborator

u16 is quite small for a huge graph

The SocketIdx only refers to the sockets on one instruction. So one instruction could hold at most 2^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

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 a String (or an opaque wrapper type thereof).

> u16 is quite small for a huge graph The `SocketIdx` only refers to the sockets on **one** instruction. So **one** instruction could hold at most `2^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 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 a `String` (or an opaque wrapper type thereof).
Author
Collaborator

I'll clarify the u16 part on the docs of SocketIdx.

I'll clarify the u16 part on the docs of `SocketIdx`.
Owner

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)

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)
Author
Collaborator

I do think that there should only be at most one value because that might make some stuff a lot more elegant

  • What one value would you want for referring to both a specific instruction and differentiating the sockets on it further would you suggest?
  • What would it make more elegant in specific?

(and this is also the wrong place to discuss this)

  • Why is it?
> I do think that there should only be at most one value because that might make some stuff a lot more elegant - What one value would you want for referring to _both_ a specific instruction and differentiating the sockets on it further would you suggest? - What would it make more elegant in specific? > (and this is also the wrong place to discuss this) - Why is it?
Owner

Why is it?

I think this discussion doesn't belong in a code review, probably in #4.

  • What one value would you want for referring to both a specific instruction and differentiating the sockets on it further would you suggest?
  • What would it make more elegant in specific?

Nevermind, I am just rambling out of my unhealthy functional programming addiction (and I really like the way nix does it)

> Why is it? I think this discussion doesn't belong in a code review, probably in #4. > - What one value would you want for referring to both a specific instruction and differentiating the sockets on it further would you suggest? > - What would it make more elegant in specific? Nevermind, I am just rambling out of my unhealthy functional programming addiction (and I really like the way nix does it)
multisamplednight marked this conversation as resolved
@ -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?")
Owner

noise is an instruction type. so are the others, we'll need more complex code to determine the correct numbers here in the future...

noise is an instruction type. so are the others, we'll need more complex code to determine the correct numbers here in the future...
multisamplednight marked this conversation as resolved
@ -0,0 +68,4 @@
pub struct Instruction(Span);
impl Instruction {
/// Where this instruction is written down.
Owner

do you mean like "written down in code"? or am i misunderstanding?

do you mean like "written down in code"? or am i misunderstanding?
Owner

This whole project screams for tests lol

This whole project screams for tests lol
Author
Collaborator

do you mean like "written down in code"? or am i misunderstanding?

No, you're correct. I'll update the docs to be more specific.

> do you mean like "written down in code"? or am i misunderstanding? No, you're correct. I'll update the docs to be more specific.
multisamplednight marked this conversation as resolved
@ -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.
Owner

A reason why that might happen is reusable blocks aka user defined functions?

A reason why that might happen is reusable blocks aka user defined functions?
Author
Collaborator

Those would be still in the same GraphIr.

Those would be still in the same `GraphIr`.
Owner

How do you intend to implement custom function calls then? Rough outline because currently I don't think I understand

gotos kinda or...?

How do you intend to implement custom function calls then? Rough outline because currently I don't think I understand `goto`s kinda or...?
Author
Collaborator

Edges to a different part of the graph. One could think of edges as being kind of gotos.

Edges to a different part of the graph. One could think of edges as being kind of `goto`s.
Owner

that doesn't solve namespacing for said functions.
and imports (which will be necessary in the future anyway), so multiple GraphIrs 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.

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.
Author
Collaborator

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 GraphIrs necessary to load libraries? Those can be added to the same, global GraphIr. This program also features 2 completely disjoint evaluation paths that don't correlate, but can still be thrown into the same GraphIr:

open 1.png | save 2.png
open 3.png | invert | save 4.png
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, global `GraphIr`. This program also features 2 completely disjoint evaluation paths that don't correlate, but can still be thrown into the _same_ `GraphIr`: ```haskell open 1.png | save 2.png open 3.png | invert | save 4.png ```
Owner

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.

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.
schrottkatze marked this conversation as resolved
@ -0,0 +159,4 @@
///
/// # Panics
///
/// Panics if there are any cycles in the IR, as it needs to be a DAG.
Owner

Should later probably more of a graceful exit...

Should later probably more of a graceful exit...
multisamplednight marked this conversation as resolved
@ -0,0 +1,90 @@
//! The midterm solution for source representation, until we've got a nice source frontent.
Owner

typo: frontend

typo: frontend
multisamplednight marked this conversation as resolved
@ -0,0 +8,4 @@
2: Write((
target: File("testfiles/inverted.png"),
format: Png,
)),
Owner

a map of numbers to instructions could also just be a vector?

a map of numbers to instructions could also just be a vector?
Author
Collaborator

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.

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.
Owner

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)

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)
Author
Collaborator

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

  1. generate a new ID
  2. or re-use an existing ID
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 1. generate a new ID 2. or re-use an existing ID
Owner

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)

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)
Author
Collaborator

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)?

open base.png    >|
open stencil.png >|
mask
|> show
|> (invert | show)
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)? ```haskell open base.png >| open stencil.png >| mask |> show |> (invert | show) ```
Owner

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.

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.
schrottkatze marked this conversation as resolved
Author
Collaborator

image licensing situation

No, I do not know the license of the juan meme image. I'd be happy for any alternative suggestions you might have though!

> image licensing situation No, I do not know the license of the juan meme image. I'd be happy for any alternative suggestions you might have though!
multisamplednight added 1 commit 2024-01-19 11:07:25 +00:00
Owner

These random rails for now? I know the license situation because well... I made it myself. Probably isn't optimal either, though.

These random rails for now? I know the license situation because well... I made it myself. Probably isn't optimal either, though.
Author
Collaborator

What license do you put it under, then?

What license do you put it under, then?
Owner

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. 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!
Author
Collaborator

CC-BY-SA 4.0?

CC-BY-SA 4.0?
multisamplednight added 1 commit 2024-01-19 13:11:53 +00:00
Author
Collaborator

@schrottkatze The latest commit included the new licenses, both of the image and the code as a whole. Are you okay with those licenses?

@schrottkatze The latest commit included the new licenses, both of the image and the code as a whole. Are you okay with those licenses?
multisamplednight added 1 commit 2024-01-19 13:27:18 +00:00
multisamplednight added 1 commit 2024-01-19 13:37:38 +00:00
Owner

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?

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?
Author
Collaborator

AGPL is lovely!

Is that explicit agreement that you're okay with both the CC BY-SA 4.0 and the AGPL?

Maybe also add a pointer to the node each time? Or do you think that'd have a significant performance impact or something?

  • Where should that pointer be stored? What are you referring to exactly?
  • What do you refer to by "node"? A socket or an instruction?
> AGPL is lovely! Is that explicit agreement that you're okay with _both_ the CC BY-SA 4.0 and the AGPL? > Maybe also add a pointer to the node each time? Or do you think that'd have a significant performance impact or something? - Where should that pointer be stored? What are you referring to exactly? - What do you refer to by "node"? A socket or an instruction?
multisamplednight force-pushed graph-ir from 3b3a3c978e to 52f8bda516 2024-01-20 14:59:50 +00:00 Compare
Owner

Is that explicit agreement that you're okay with both the CC BY-SA 4.0 and the AGPL?

Yes. The image should be under CC-BY-SA 4.0 and my code under the AGPL.

What do you refer to by "node"? A socket or an instruction?

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

> Is that explicit agreement that you're okay with both the CC BY-SA 4.0 and the AGPL? Yes. The image should be under CC-BY-SA 4.0 and my code under the AGPL. > What do you refer to by "node"? A socket or an instruction? 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
Author
Collaborator

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 a Socket. It only exists as a newtype to avoid accidental modification of the contained usize (which we'll need to change for named sockets anyway).

> 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 a `Socket`. It only exists as a newtype to avoid accidental modification of the contained `usize` (which we'll need to change for named sockets anyway).
Owner

ahh I see!

ahh I see!
multisamplednight added 1 commit 2024-01-20 17:03:48 +00:00
multisamplednight force-pushed graph-ir from efb83c3796 to c36a17d313 2024-01-20 17:33:59 +00:00 Compare
multisamplednight force-pushed graph-ir from c36a17d313 to 98c9399ee4 2024-01-20 20:13:21 +00:00 Compare
multisamplednight added 1 commit 2024-01-20 20:32:48 +00:00
multisamplednight added 2 commits 2024-01-20 20:47:56 +00:00
multisamplednight added 1 commit 2024-01-21 02:23:17 +00:00
multisamplednight added 1 commit 2024-01-21 03:22:35 +00:00
Very hacky, but this is enough to be finished with the graph IR for now.
multisamplednight requested review from schrottkatze 2024-01-21 03:22:43 +00:00
multisamplednight added 1 commit 2024-01-21 03:24:09 +00:00
multisamplednight changed title from WIP: feat: Graph IR to feat: Graph IR 2024-01-21 03:27:03 +00:00
Author
Collaborator

Done. The eval impl is hacky, but it works for a quick demo of how to use the graph IR.

Done. The eval impl is hacky, but it works for a quick demo of how to use the graph IR.
schrottkatze reviewed 2024-01-21 19:25:27 +00:00
schrottkatze left a comment
Owner

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.

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))),
Owner

I generally prefer using to_owned, since it's more clear that you're turning an unowned string (&str) into an owned String.

-            e => Err(Config::UnknownExtension(e.map(str::to_string))),
+            e => Err(Config::UnknownExtension(e.map(str::to_owned))),
I generally prefer using `to_owned`, since it's more clear that you're turning an unowned string (`&str`) into an owned `String`. ```diff - e => Err(Config::UnknownExtension(e.map(str::to_string))), + e => Err(Config::UnknownExtension(e.map(str::to_owned))), ```
multisamplednight marked this conversation as resolved
@ -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");
Owner

i mean yes, maybe slap one (or two) TODO comments there?

i mean yes, maybe slap one (or two) `TODO` comments there?
multisamplednight marked this conversation as resolved
@ -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
Owner

see note in Evaluator trait above this method

meow? is this referring to the note from line 14-16 in the struct?

> see note in Evaluator trait above this method meow? is this referring to the note from line 14-16 in the struct?
Author
Collaborator

No. It is referring to the Evaluator trait at crates/eval/src/lib.rs:9 and to the comment on line 12f.

No. It is referring to the `Evaluator` **trait** at `crates/eval/src/lib.rs:9` and to the comment on line 12f.
Author
Collaborator

Actually, if it causes this much confusion, I'll just remove the comment here to make the other one the single source of truth.

Actually, if it causes this much confusion, I'll just remove the comment here to make the other one the single source of truth.
multisamplednight marked this conversation as resolved
@ -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
Owner

wdym?
but yes, we should add more output methods.

wdym? but yes, we should add more output methods.
Author
Collaborator

There will necessarily be more variants added to Variant later on, so the lint is suppressed since the if let will be necessary then. The comment refers to this circumstance, not to the output methods (whatever you may mean with that).

There will necessarily be more variants added to `Variant` later on, so the lint is suppressed since the `if let` will be necessary then. The comment refers to this circumstance, **not** to the output methods (whatever you may mean with that).
Owner

Ahh okay! I meant more sinks/consumers

and potentially for write to also be able to write csvs etc

Ahh okay! I meant more sinks/consumers and potentially for write to also be able to write csvs etc
schrottkatze marked this conversation as resolved
@ -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
Owner

Is this the note that was referred to in the other file? Or the one inside the struct?

Is this the note that was referred to in the other file? Or the one inside the struct?
Author
Collaborator

It is referred to by crates/eval/src/kind/debug/mod.rs:22.

It is referred to by `crates/eval/src/kind/debug/mod.rs:22`.
multisamplednight marked this conversation as resolved
@ -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
Owner

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.

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.
Author
Collaborator

I'm going to rework this to be much more statically typed, but for now this should be fine.

Sure, however you plan on implementing that, since the evaluation cache of the evaluator needs to store something.

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

I do not see how this is related to Variant at all.

> I'm going to rework this to be much more statically typed, but for now this should be fine. Sure, however you plan on implementing that, since the evaluation cache of the evaluator needs to store _something_. > 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 I do not see how this is related to `Variant` at all.
Owner

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.

> 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.
schrottkatze marked this conversation as resolved
@ -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.
Owner

note to keep in mind for later, maybe this should also have the option to have default values? probably rather relevant for frontends...

note to keep in mind for later, maybe this should also have the option to have default values? probably rather relevant for frontends...
Author
Collaborator

Could you possibly give an example where a default value on an instruction would be desirable?

Could you possibly give an example where a default value on an instruction would be desirable?
Owner

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

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
Author
Collaborator

Named args are implied by #4.

Named args are implied by #4.
multisamplednight marked this conversation as resolved
@ -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.
Owner

potentially clarify that that's just for us, the devs, debugging?

potentially clarify that that's just for us, the devs, debugging?
Author
Collaborator

That's why it is in a doc comment.

That's why it is in a doc comment.
Owner

hm yes i am big brain sorrry ^^'

hm yes i am big brain sorrry ^^'
schrottkatze marked this conversation as resolved
@ -0,0 +1,86 @@
//! The midterm solution for source representation, until we've got a nice source frontend.
Owner

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.

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.
Author
Collaborator

I plan to get rid of this as soon as we have a meaningful sourceir pipeline, actually.

I plan to get rid of this as soon as we have a meaningful `source` → `ir` pipeline, actually.
Owner

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 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?)
Author
Collaborator

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.

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
Owner

Ok seriously we probably need to have a meeting and talk about the terminology ^^' this is really becoming a headache and a mess

Ok seriously we probably need to have a meeting and talk about the terminology ^^' this is really becoming a headache and a mess
Author
Collaborator

Ok seriously we probably need to have a meeting and talk about the terminology

Anytime.

this is really becoming a headache and a mess

Assumed somewhere behind this statement is a request for a action: Should I add a short overview of terms in the design doc?

> Ok seriously we probably need to have a meeting and talk about the terminology Anytime. > this is really becoming a headache and a mess Assumed somewhere behind this statement is a request for a action: Should I add a short overview of terms in the design doc?
Author
Collaborator

I'll just do that, actually.

I'll just do that, actually.
Author
Collaborator

Did that in 63d7993940 on page 2 of the design doc. Could you possibly look over it and see if it satisfies your desires?

Did that in https://forge.katzen.cafe/katzen-cafe/iowo/commit/63d79939404679181fe28d11bcf9836dbc96eb4a on page 2 of the design doc. Could you possibly look over it and see if it satisfies your desires?
Owner

Ask schrottkatze on why the differentiation is important.

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.

> Ask schrottkatze on why the differentiation is important. 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.
Author
Collaborator

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,

open lol.png | invert | save idk.png

has one major pipeline (everything), but actually 2 more pipelines: open lol.png | invert and invert | save idk.png. Do you want to remove those two and make a new term "subpipeline"? If so, why?

> 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, ```hs open lol.png | invert | save idk.png ``` has one major pipeline (everything), but actually 2 more pipelines: `open lol.png | invert` and `invert | save idk.png`. Do you want to remove those two and make a new term "subpipeline"? If so, why?
multisamplednight added 3 commits 2024-01-21 20:29:28 +00:00
multisamplednight added 1 commit 2024-01-21 21:12:18 +00:00
schrottkatze requested changes 2024-01-22 08:20:09 +00:00
schrottkatze left a comment
Owner

Also, I propose you add Nix and direnv to the CONTRIBUTING.mds tech stack list.

Potentially with a note how to use the flake and envrc?

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:
Owner

Lets add an alias for the docs recipe that's just doc, since I keep typing that out of habit.
Makes it simpler for everyone to use whatever they want.

Lets add an alias for the `docs` recipe that's just `doc`, since I keep typing that out of habit. Makes it simpler for everyone to use whatever they want.
Author
Collaborator

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 through

# may need to replace origin with upstream if upstream points to the main repo instead
git fetch origin pull/6/head:graph-ir-old
git switch graph-ir-old

EDIT: After conversing with Schrottkatze locally, I decided to continue contributing under some conditions.

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 through ```sh # may need to replace origin with upstream if upstream points to the main repo instead git fetch origin pull/6/head:graph-ir-old git switch graph-ir-old ``` EDIT: After conversing with Schrottkatze locally, I decided to continue contributing under some conditions.

Pull request closed

Sign in to join this conversation.
No reviewers
No labels
proposal
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: katzen-cafe/iowo#6
No description provided.