fixstars / ion-kit

Modernized graph-based data processing framework
MIT License
7 stars 6 forks source link

Feature/ python graph API and strongly typed ID #252

Closed xinyuli1204 closed 4 months ago

xinyuli1204 commented 4 months ago
  1. fix duplicate param name error when combine two graphs with same input name.
  2. c_api for graph In short words, argumentname now becomes `std::string s = "" + nodeid + "" + name + std::tostring(index) + "" + graph_id;`
iitaku commented 4 months ago

@xinyuli1204 Basically looks good.

Now we have three different id (Node, Port and now Graph is added) and it has same type (std::string) even if these are not compatible each other. Can you think about introducing strongly typed alias for each id? ref: https://stackoverflow.com/questions/34287842/strongly-typed-using-and-typedef

xinyuli1204 commented 4 months ago

@xinyuli1204 Basically looks good.

Now we have three different id (Node, Port and now Graph is added) and it has same type (std::string) even if these are not compatible each other. Can you think about introducing strongly typed alias for each id? ref: https://stackoverflow.com/questions/34287842/strongly-typed-using-and-typedef

@iitaku done using NodeID = StringID; using GraphID = StringID; using PortID = StringID;

xinyuli1204 commented 4 months ago

@iitaku can I ask something about pred_chan in port.h? From my understanding pred_chan makes tuple of NodeId and string std::tuple<NodeID, std::string>;, and pred_id() will get previous NodeID , but when constructing new port in port.cc

Port::Impl::Impl(const std::string& pid, const std::string& pn, const Halide::Type& t, int32_t d, const GraphID & gid)
    : id(sole::uuid4().str()), pred_chan{pid, pn}, succ_chans{}, type(t), dimensions(d), graph_id(gid)

pred_chan{pid, pn}, why it takes pid instead?

I wonder if port should init like this

Port::Impl::Impl(const NodeID & nid, const std::string& pn, const Halide::Type& t, int32_t d, const GraphID & gid)
    : id(sole::uuid4().str()), pred_chan{nid, pn}, succ_chans{}, type(t), dimensions(d), graph_id(gid)
{
    params[0] = Halide::Internal::Parameter(type, dimensions != 0, dimensions, argument_name(nid, pn, 0, gid));
}

and is PortId really needed? I didn't find it is used in other place but we can keep it as well

Fixstars-iizuka commented 4 months ago

@xinyuli1204 It's correct. pid is actually nid.

PortID is not used at this moment, but if it has no downside, let's leave it as it is for the future.