WIP: image processing library (or libraries?) #12

Draft
schrottkatze wants to merge 15 commits from schrottkatze/iowo:proc-libs into main
9 changed files with 93 additions and 37 deletions
Showing only changes of commit 0ebfed66ed - Show all commits

View file

@ -1,3 +1,2 @@
pub mod enum_based;
pub mod trait_based;

View file

@ -1,3 +1,9 @@
//! An experiment for a hyper-modular trait-based architecture.
//!
//! Patterns defining this (or well, which I reference a lot while writing this):
//! - [Command pattern using trait objects](https://rust-unofficial.github.io/patterns/patterns/behavioural/command.html)
//! - [Builder pattern](https://rust-unofficial.github.io/patterns/patterns/creational/builder.html)
pub mod data;
pub mod element;
pub mod ops;

View file

@ -1,2 +1,7 @@
//! Definitions of the data transfer and storage types.
/// Types for element and pipeline IO
schrottkatze marked this conversation as resolved Outdated

Given that you use a module comment at the very start of this file, it seems like these doc-comments would also belong into their respective modules.
This gives the reader added context when opening the respective module e.g. after following a type definition.

Given that you use a module comment at the very start of this file, it seems like these doc-comments would also belong into their respective modules. This gives the reader added context when opening the respective module e.g. after following a type definition.
pub mod io;
/// Raw data types contained in `io`
schrottkatze marked this conversation as resolved Outdated

Could use an intralink.

-/// Raw data types contained in `io`
+/// Raw data types contained in [`io`]
Could use an intralink. ```diff -/// Raw data types contained in `io` +/// Raw data types contained in [`io`] ```
pub mod raw;

View file

@ -1,29 +1,38 @@
use super::raw::{Data, OwnedData};
/// Newtype struct with borrowed types for pipeline/element inputs, so that doesn't force a move or clone
pub struct Inputs<'a>(Vec<Data<'a>>);
impl<'a> Inputs<'a> {
/// get inner value(s)
schrottkatze marked this conversation as resolved

Quite ambiguous doc-comment also regarding the rather lengthy doc-comment on the type itself.
How about removing this method altogether and making the content of Inputs directly public,
given that one's free to convert from/to it already?

Quite ambiguous doc-comment also regarding the rather lengthy doc-comment on the type itself. How about removing this method altogether and making the content of `Inputs` directly public, given that one's free to convert from/to it already?
pub(crate) fn inner(&self) -> Vec<Data<'a>> {
self.0.clone()
}
}
impl<'a> From<Vec<Data<'a>>> for Inputs<'a> {
fn from(value: Vec<Data<'a>>) -> Self {
Self(value)
}
}
impl<'a, T: Into<Data<'a>>> From<T> for Inputs<'a> {
fn from(value: T) -> Self {
Self(vec![value.into()])
}
}
impl<'a> From<&'a Outputs> for Inputs<'a> {
fn from(value: &'a Outputs) -> Self {
Self(value.0.iter().map(std::convert::Into::into).collect())
schrottkatze marked this conversation as resolved

Unnecessary full method path, consider just using From::from or Into::into instead.

Unnecessary full method path, consider just using `From::from` or `Into::into` instead.
Review

ah yes, rust-analyzer loves completing full paths lol

ah yes, rust-analyzer loves completing full paths lol
}
}
/// Newtype struct around `OwnedData` for pipeline/element outputs
schrottkatze marked this conversation as resolved Outdated

I already see that this is a newtype around OwnedData, just re-stating it does not add semantical value. Rather, what would this struct be used for?

I already see that this is a newtype around `OwnedData`, just re-stating it does not add semantical value. Rather, what would this struct be used for?
pub struct Outputs(Vec<OwnedData>);
impl Outputs {
/// consume self and return inner value(s)
pub fn into_inner(self) -> Vec<OwnedData> {
schrottkatze marked this conversation as resolved

Wait, why is Outputs allowed to be consumed for its inner content consumed while Inputs doesn't?

Wait, why is `Outputs` allowed to be consumed for its inner content consumed while `Inputs` doesn't?
Review

Inputs only contains a Vec of Data which either contains a string slice or an integer, which are really cheap to clone. OwnedData would be much heavier to clone in this case.

(I'm currently not happy how the IO of instructions works anyway, planning on reworking that to be more sensible, clear and flexible))

`Inputs` only contains a `Vec` of `Data` which either contains a string slice or an integer, which are really cheap to clone. `OwnedData` would be much heavier to clone in this case. (I'm currently not happy how the IO of instructions works anyway, planning on reworking that to be more sensible, clear and flexible))
self.0
}

View file

@ -1,3 +1,25 @@
//! Dynamic data storage and transfer types
/// Owned data type, for use mostly in outputs and storage
#[derive(Clone, Debug)]
pub enum OwnedData {
schrottkatze marked this conversation as resolved Outdated

Given the common pattern of AsRef in the stdlib and InstructionRef already being used in this project, how about renaming OwnedData → Data and Data below → DataRef?

Given the common pattern of `AsRef` in the stdlib and `InstructionRef` already being used in this project, how about renaming `OwnedData` → `Data` and `Data` below → `DataRef`?

did the renaming, gonna take care of the AsRef impl later (I remember trying that earlier and something causing problems, i forgot what that was though.)

did the renaming, gonna take care of the `AsRef` impl later (I remember trying that earlier and something causing problems, i forgot what that was though.)
String(String),
Int(i32),
}
impl From<String> for OwnedData {
fn from(value: String) -> Self {
Self::String(value)
}
}
impl From<i32> for OwnedData {
fn from(value: i32) -> Self {
Self::Int(value)
}
}
/// Unowned data type, for inputs into runner functions
#[derive(Clone, Copy)]
schrottkatze marked this conversation as resolved Outdated

Why does OwnedData implement Debug while Data doesn't?

Why does `OwnedData` implement `Debug` while `Data` doesn't?
pub enum Data<'a> {
String(&'a str),
@ -5,6 +27,7 @@ pub enum Data<'a> {
}
impl Data<'_> {
/// converts itself to `OwnedData`
pub fn to_owned_data(&self) -> OwnedData {
schrottkatze marked this conversation as resolved Outdated

Consider implementing ToOwned and AsRef appropiately instead.

OT: Wondering that clippy didn't scream about this. Interesting, since it has a lint for this, but apparently it didn't trigger since the name was (imo unnecessarily) suffixed with _data.

Consider implementing [`ToOwned`] and [`AsRef`] appropiately instead. OT: Wondering that clippy didn't scream about this. Interesting, since [it has a lint for this](https://rust-lang.github.io/rust-clippy/master/index.html#/should_implement_trait), but apparently it didn't trigger since the name was (imo unnecessarily) suffixed with `_data`. [`ToOwned`]: https://doc.rust-lang.org/std/borrow/trait.ToOwned.html [`AsRef`]: https://doc.rust-lang.org/std/convert/trait.AsRef.html

I did that deliberately to not trigger the lint, but I'll fix that and properly do it after fixing your simpler reviews

I did that deliberately to not trigger the lint, but I'll fix that and properly do it after fixing your simpler reviews

As I've just realized because I was trying to implement ToOwned/AsRef, it isn't even necessary to have a seperate DataRef, so i removed that entirely and... who would've thought, it's much more readable and concise overall now

As I've just realized because I was trying to implement `ToOwned`/`AsRef`, it isn't even necessary to have a seperate `DataRef`, so i removed that entirely and... who would've thought, it's much more readable and concise overall now
match self {
Data::String(s) => (*s).to_owned().into(),
@ -12,16 +35,19 @@ impl Data<'_> {
}
}
}
impl<'a> From<&'a str> for Data<'a> {
fn from(value: &'a str) -> Self {
Self::String(value)
}
}
impl From<i32> for Data<'_> {
fn from(value: i32) -> Self {
Self::Int(value)
}
}
impl<'a> From<&'a OwnedData> for Data<'a> {
fn from(value: &'a OwnedData) -> Self {
match value {
@ -30,19 +56,3 @@ impl<'a> From<&'a OwnedData> for Data<'a> {
}
}
}
#[derive(Clone, Debug)]
pub enum OwnedData {
String(String),
Int(i32),
}
impl From<String> for OwnedData {
fn from(value: String) -> Self {
Self::String(value)
}
}
impl From<i32> for OwnedData {
fn from(value: i32) -> Self {
Self::Int(value)
}
}

View file

@ -1,17 +1,23 @@
//! The trait and type representations
use crate::experimental::trait_based::data::io::Inputs;
use super::data::io::Outputs;
pub(crate) trait PipelineElement {
/// return a static runner function pointer to avoid dynamic dispatch during pipeline execution - Types MUST match the signature

The condition might be more visible if it's put in an explicit # Indirect panics section or the like.

The condition might be more visible if it's put in an explicit `# Indirect panics` section or the like.

probably gonna refactor this to not panic in the future and add proper handling (Results lol)

probably gonna refactor this to not panic in the future and add proper handling (`Result`s lol)
fn runner(&self) -> fn(&Inputs) -> Outputs;
fn signature(&self) -> ElementIo;
/// return the signature of the element
fn signature(&self) -> ElementSignature;
}
pub(crate) struct ElementIo {
/// Type signature for an element used for static checking
pub(crate) struct ElementSignature {
pub inputs: Vec<DataType>,
pub outputs: Vec<DataType>,
}
/// Data type enum
#[derive(Clone, Copy, PartialEq, Eq, Debug)]
pub enum DataType {
String,

View file

@ -1,3 +1,4 @@
//! Operations on numeric data
use core::panic;
use crate::experimental::trait_based::{
@ -5,9 +6,10 @@ use crate::experimental::trait_based::{
io::{Inputs, Outputs},
raw::Data,
},
element::{DataType, ElementIo, PipelineElement},
element::{DataType, ElementSignature, PipelineElement},
};
/// Addition
pub struct Add(pub i32);

Why do Add, Subtract, Concatenate hold a pub i32 if it's never used?

Why do `Add`, `Subtract`, `Concatenate` hold a `pub i32` if it's never used?

because i wanted to do second params like that originally, then realizing it doesn't work that "easily". so basically, still WIP

because i wanted to do second params like that originally, then realizing it doesn't work that "easily". so basically, still WIP
impl PipelineElement for Add {
fn runner(&self) -> fn(&Inputs) -> Outputs {
@ -20,14 +22,15 @@ impl PipelineElement for Add {
}
}
fn signature(&self) -> ElementIo {
ElementIo {
fn signature(&self) -> ElementSignature {
ElementSignature {
inputs: vec![DataType::Int, DataType::Int],
outputs: vec![DataType::Int],
}
}
}
/// Subtraction
pub struct Subtract(pub i32);
impl PipelineElement for Subtract {
fn runner(&self) -> fn(&Inputs) -> Outputs {
@ -40,14 +43,15 @@ impl PipelineElement for Subtract {
}
}
fn signature(&self) -> ElementIo {
ElementIo {
fn signature(&self) -> ElementSignature {
ElementSignature {
inputs: vec![DataType::Int, DataType::Int],
outputs: vec![DataType::Int],
}
}
}
/// Turn input to string
pub struct Stringify;
impl PipelineElement for Stringify {
fn runner(&self) -> fn(&Inputs) -> Outputs {
@ -60,8 +64,8 @@ impl PipelineElement for Stringify {
}
}
fn signature(&self) -> ElementIo {
ElementIo {
fn signature(&self) -> ElementSignature {
ElementSignature {
inputs: vec![DataType::Int],
outputs: vec![DataType::String],
}

View file

@ -1,11 +1,13 @@
//! Operation on String/text data
use crate::experimental::trait_based::{
data::{
io::{Inputs, Outputs},
raw::Data,
},
element::{DataType, ElementIo, PipelineElement},
element::{DataType, ElementSignature, PipelineElement},
};
/// Concatenate the inputs
pub struct Concatenate(pub String);
impl PipelineElement for Concatenate {
fn runner(&self) -> fn(&Inputs) -> Outputs {
@ -18,14 +20,15 @@ impl PipelineElement for Concatenate {
}
}
fn signature(&self) -> ElementIo {
ElementIo {
fn signature(&self) -> ElementSignature {
ElementSignature {
inputs: vec![DataType::String, DataType::String],
outputs: vec![DataType::String],
}
}
}
/// Turn input text to uppercase
pub struct Upper;
impl PipelineElement for Upper {
fn runner(&self) -> fn(&Inputs) -> Outputs {
@ -38,14 +41,15 @@ impl PipelineElement for Upper {
}
}
fn signature(&self) -> ElementIo {
ElementIo {
fn signature(&self) -> ElementSignature {
ElementSignature {
inputs: vec![DataType::String],
outputs: vec![DataType::String],
}
}
}
/// Turn input text to lowercase
pub struct Lower;
impl PipelineElement for Lower {
fn runner(&self) -> fn(&Inputs) -> Outputs {
@ -58,8 +62,8 @@ impl PipelineElement for Lower {
}
}
fn signature(&self) -> ElementIo {
ElementIo {
fn signature(&self) -> ElementSignature {
ElementSignature {
inputs: vec![DataType::String],
outputs: vec![DataType::String],
}

View file

@ -2,21 +2,25 @@ use super::data::io::{Inputs, Outputs};
use super::element::PipelineElement;
use super::ops::prelude::*;
// TODO:
// - Bind additional inputs if instruction has more then one and is passd without any additional
// - allow binding to pointers to other pipelines?
// - allow referencing earlier data
/// Builder for the pipelines that are actually run
///
/// TODO:
/// - Bind additional inputs if instruction has more then one and is passd without any additional
/// - allow binding to pointers to other pipelines?
/// - allow referencing earlier data
multisamplednight marked this conversation as resolved

Those TODO:s seem like they should belong in an issue, so one can

  • discuss on them
  • selectively mark them as done/undone
  • edit them easily
  • link them from a respective PR
Those `TODO:`s seem like they should belong in an issue, so one can - discuss on them - selectively mark them as done/undone - edit them easily - link them from a respective PR
Review

this is heavily WIP, remember, these are experiments. There will not be a seperate PR until this is not only out of experimental state, but a functioning image processing library that has all that fixed.

this is heavily WIP, remember, these are experiments. There will not be a seperate PR until this is not only out of experimental state, but a functioning image processing library that has all that fixed.

So you'd rather do all the work of finding all TODO:s and converting them to issues (and then creating a PR removing them) after "finishing" prowocessing?

So you'd rather do all the work of finding all `TODO:`s and converting them to issues (and then creating a PR removing them) after "finishing" `prowocessing`?

Also, that just addresses one point, while the other 3 are still standing.

Also, that just addresses one point, while the other 3 are still standing.
Review

The intention is to perfect the api beforehand, and tbh, you'll be the only other person reviewing stuff anyway. And wdym "editing them easily", tbh i find editing text in my editor much, much easier then working with a forgejo web ui...

and if they're done, i delete them.

The intention is to perfect the api beforehand, and tbh, you'll be the only other person reviewing stuff anyway. And wdym "editing them easily", tbh i find editing text in my editor much, *much* easier then working with a forgejo web ui... and if they're done, i delete them.

Sounds fine to me.

Sounds fine to me.
pub struct PipelineBuilder {
elements: Vec<Box<dyn PipelineElement>>,
}
impl PipelineBuilder {
/// Create new, empty builder
pub fn new() -> Self {
Self {
elements: Vec::new(),
}
}
/// Insert element into pipeline
fn insert<T: PipelineElement + 'static>(mut self, el: T) -> Self {
if let Some(previous_item) = self.elements.last() {
assert_eq!(
@ -28,21 +32,25 @@ impl PipelineBuilder {
self
}
/// insert string concatenattion element
#[must_use]
pub fn concatenate(self, sec: String) -> Self {
self.insert(Concatenate(sec))
}
/// insert string uppercase element
#[must_use]
pub fn upper(self) -> Self {
self.insert(Upper)
}
/// insert string lowercase element
#[must_use]
pub fn lower(self) -> Self {
self.insert(Lower)
}
/// insert numeric addition element
#[must_use]
#[allow(
clippy::should_implement_trait,
@ -52,16 +60,19 @@ impl PipelineBuilder {
self.insert(Add(sec))
}
/// insert numeric subtraction element
#[must_use]
pub fn subtract(self, sec: i32) -> Self {
self.insert(Subtract(sec))
}
/// insert stringify element
#[must_use]
pub fn stringify(self) -> Self {
self.insert(Stringify)
}
/// Build the pipeline. Doesn't check again - `insert` should verify correctness.
pub fn build(&self) -> Pipeline {
let mut r = Vec::new();
@ -77,11 +88,13 @@ impl Default for PipelineBuilder {
}
}
/// Runnable pipeline - at the core of this library
pub struct Pipeline {
runners: Vec<fn(&Inputs) -> Outputs>,

Also regarding the enum-based arch: Why the indirection of fn(&Inputs) -> Outputs? Why does Pipeline not hold Box<dyn Element> as well?

Also regarding the enum-based arch: Why the indirection of `fn(&Inputs) -> Outputs`? Why does `Pipeline` not hold `Box<dyn Element>` as well?
Review

Unless I misunderstood rusts dyn, this avoids the overhead of dynamic dispatch by saving static function pointers, which can just be called on the fly

Unless I misunderstood rusts `dyn`, this avoids the overhead of dynamic dispatch by saving static function pointers, which can just be called on the fly

They do save 1 indirection, but that is 1 indirection which hasn't even been benchmarked yet against

  1. worse debuggability
    • when Debugging Pipeline if it were to implement Debug, all one sees at the moment is a bunch of addresses of a function in hexadecimal, e.g. 0x000056380fa85320.
  2. when writing fn runner() as opposed to a direct fn eval(&Inputs) -> Outputs or the like
    1. an extra indent for the actual logic
    2. extra noise for defining the return type and the returned closure

Sidenote: If you care this much about indirection, &Inputs is actually &Vec<&Data>, which are 2 indirections already before the element can access any contained data.

They do save 1 indirection, but that is 1 indirection which hasn't even been benchmarked yet against 1. worse debuggability - when `Debug`ging `Pipeline` if it were to implement `Debug`, all one sees at the moment is a bunch of addresses of a function in hexadecimal, e.g. `0x000056380fa85320`. 2. when writing `fn runner()` as opposed to a direct `fn eval(&Inputs) -> Outputs` or the like 1. an extra indent for the actual logic 2. extra noise for defining the return type and the returned closure Sidenote: If you care this much about indirection, `&Inputs` is actually `&Vec<&Data>`, which are 2 indirections already before the element can access any contained data.
Review
  1. worse debuggability

i have an idea what one could do for that, I'll be implementing that soon-ish when i have the energy

  1. when writing fn runner() as opposed to a direct fn eval(&Inputs) -> Outputs or the like
    1. an extra indent for the actual logic
    2. extra noise for defining the return type and the returned closure

the return types are defined as opaque types deliberately, since during actual execution the pipeline does not know those.
the only thing the pipeline is supposed to do, is to execute the runners in order and get data where it's supposed to go (which is, once again, an unsolved problem currently)

Sidenote: If you care this much about indirection, &Inputs is actually &Vec<&Data>, which are 2 indirections already before the element can access any contained data.

yes, but I don't think that's avoidable.

> 1. worse debuggability i have an idea what one could do for that, I'll be implementing that soon-ish when i have the energy > 1. when writing fn runner() as opposed to a direct fn eval(&Inputs) -> Outputs or the like > 1. an extra indent for the actual logic > 2. extra noise for defining the return type and the returned closure the return types are defined as opaque types deliberately, since during actual execution the pipeline does not know those. the only thing the pipeline is supposed to do, is to execute the runners in order and get data where it's supposed to go (which is, once again, an unsolved problem currently) > Sidenote: If you care this much about indirection, &Inputs is actually &Vec<&Data>, which are 2 indirections already before the element can access any contained data. yes, but I don't think that's avoidable.

i have an idea what one could do for that, I'll be implementing that soon-ish when i have the energy

Does this idea imply a map of fn pointer to debug info? If so, I'm not sure if the size increase outweighs the 1 (still unbenchmarked) indirection.

the return types are defined as opaque types deliberately, since during actual execution the pipeline does not know those.
the only thing the pipeline is supposed to do, is to execute the runners in order and get data where it's supposed to go (which is, once again, an unsolved problem currently)

That does not address the points you quote at all. At no point I questioned the fn pointers being opaque. You just re-stated what the pipeline should do, which I already know, without addressing what I listed.

> i have an idea what one could do for that, I'll be implementing that soon-ish when i have the energy Does this idea imply a map of fn pointer to debug info? If so, I'm not sure if the size increase outweighs the 1 (still unbenchmarked) indirection. > the return types are defined as opaque types deliberately, since during actual execution the pipeline does not know those. > the only thing the pipeline is supposed to do, is to execute the runners in order and get data where it's supposed to go (which is, once again, an unsolved problem currently) That does not address the points you quote at all. At no point I questioned the fn pointers being opaque. You just re-stated what the pipeline should do, which I already know, without addressing what I listed.

yes, but I don't think that's avoidable.

It is. One could clone the vec on every instruction call.

EDIT: Thinking about it, actually the vec will need to be created for every instruction call anyway, since in a graph, multiple instructions may be source for an instruction.

> yes, but I don't think that's avoidable. It is. One could clone the vec on every instruction call. EDIT: Thinking about it, actually the vec will need to be created for every instruction call anyway, since in a graph, multiple instructions may be source for an instruction.
}
impl Pipeline {
/// run the pipeline
pub fn run(&self, inputs: Inputs) -> Outputs {
let mut out: Outputs = inputs.into();