Feature/ python graph API and strongly typed ID #252

xinyuli1204 · 2024-02-28T23:57:54Z

fix duplicate param name error when combine two graphs with same input name.
c_api for graph
In short words,
argument_name now becomes std::string s = "_" + node_id + "_" + name + std::to_string(index) + "_" + graph_id;

iitaku · 2024-02-29T20:44:10Z

@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 · 2024-02-29T20:47:17Z

include/ion/c_ion.h

+int ion_graph_add_node(ion_graph_t, const char*, ion_node_t *);
+int ion_graph_destroy(ion_graph_t);
+int ion_graph_run(ion_graph_t);
+int ion_graph_create_with_multiple(ion_graph_t *, ion_graph_t obj1, ion_graph_t obj2);


I'm considering more straitforward naming. e.g. ion_graph_create_from_2 ? Do you have any better idea?

iitaku · 2024-02-29T20:49:20Z

include/ion/node.h

+
+
+    template<typename... Args>
+    std::vector<Port> create_ports_vector(Args... args) const {


Can you please clarify necessity of the wrapper function?

Sorry for the confusion, this is unnecessary and I delete it

xinyuli1204 · 2024-03-04T18:09:35Z

@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<node_tag>;
using GraphID = StringID<graph_tag>;
using PortID = StringID<port_tag>;

iitaku · 2024-03-05T22:47:33Z

include/ion/node.h

        Impl(const std::string& id_, const std::string& name_, const Halide::Target& target_);
+        Impl(const std::string& id_, const std::string& name_, const Halide::Target& target_, const GraphID &graph_id_);


First argument is better to be NodeID, not std::string

iitaku · 2024-03-05T22:49:38Z

include/ion/node.h

@@ -126,6 +131,40 @@ class Node {
    {
    }

+    Node(const std::string& id, const std::string& name, const Halide::Target& target, const GraphID& graph_id)


Same as above (NodeID)

iitaku · 2024-03-05T22:50:08Z

include/ion/port.h

@@ -64,12 +65,12 @@ class Port {
        std::unordered_map<uint32_t, const void *> instances;

        Impl();
-        Impl(const std::string& pid, const std::string& pn, const Halide::Type& t, int32_t d);
+        Impl(const std::string& pid, const std::string& pn, const Halide::Type& t, int32_t d, const GraphID &gid );


PortID instead of std::string

iitaku · 2024-03-05T22:51:05Z

src/port.cc

 {
-    params[0] = Halide::Internal::Parameter(type, dimensions != 0, dimensions, argument_name(pid, pn, 0));
+    params[0] = Halide::Internal::Parameter(type, dimensions != 0, dimensions, argument_name(pid, pn, 0, gid.value()));
 }

 void Port::determine_succ(const std::string& nid, const std::string& old_pn, const std::string& new_pn) {


NodeID instead of std::string

iitaku · 2024-03-05T22:53:13Z

include/ion/port.h

-    bool has_pred() const { return !std::get<0>(impl_->pred_chan).empty(); }
-    bool has_pred_by_nid(const std::string& nid) const { return !std::get<0>(impl_->pred_chan).empty(); }
+    bool has_pred() const { return !std::get<0>(impl_->pred_chan).value().empty(); }
+    bool has_pred_by_nid(const std::string& nid) const { return !to_string(std::get<0>(impl_->pred_chan)).empty(); }
    bool has_succ() const { return !impl_->succ_chans.empty(); }
    bool has_succ(const Channel& c) const { return impl_->succ_chans.count(c); }
    bool has_succ_by_nid(const std::string& nid) const {


NodeID instead of std::string

iitaku · 2024-03-05T23:04:23Z

include/ion/node.h

+    {
+    }
+
+    Port get_iport(Port arg) const {


Probably make_iport is less confusing for this internal API.

iitaku · 2024-03-05T23:05:14Z

include/ion/port.h

@@ -78,58 +79,87 @@ class Port {
     * @arg k: The key of the port which should be matched with BuildingBlock Input/Output name.
     * @arg t: The type of the value.
     */
-    Port(const std::string& n, Halide::Type t) : impl_(new Impl("", n, t, 0)), index_(-1) {}
+    Port(const std::string& n, Halide::Type t) : impl_(new Impl("", n, t, 0,  GraphID(""))), index_(-1) {}


Is GraphID("") and GraphID() identical? Let's use either one if there is no meaning for these two use

iitaku · 2024-03-05T23:07:52Z

include/ion/port.h

@@ -220,7 +250,7 @@ class Port {
                 args.push_back(Halide::Var::implicit(i));
                 args_expr.push_back(Halide::Var::implicit(i));
             }
-             Halide::Func f(param.type(), param.dimensions(), argument_name(pred_id(), pred_name(), i) + "_im");
+             Halide::Func f(param.type(), param.dimensions(), argument_name(pred_id(), pred_name(), i, graph_id_to_string()) + "_im");


We can pass GraphID to argument_name directly

iitaku · 2024-03-05T23:08:19Z

include/ion/port_map.h

@@ -16,7 +16,7 @@ class PortMap {
    template<typename T>
    [[deprecated("Port::bind can be used instead of PortMap.")]]
    void set(Port port, T v) {
-        auto& buf(scalar_buffer_[argument_name(port.pred_id(), port.pred_name(), port.index())]);
+        auto& buf(scalar_buffer_[argument_name(port.pred_id_to_string(), port.pred_name(), port.index(), port.graph_id_to_string())]);


argument name takes NodeID and GraphID, not string

xinyuli1204 · 2024-03-05T23:39:35Z

@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 · 2024-03-06T07:22:41Z

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

Xinyu Li added 5 commits February 28, 2024 10:02

graph api

b5355e3

minimum test to reproduce the error

d858f1b

add graph id when generating instance

91358a9

change test

07e06c6

fix halide buffer input

5695991

xinyuli1204 marked this pull request as ready for review February 29, 2024 18:29

modify test

9b7dbe3

iitaku reviewed Feb 29, 2024

View reviewed changes

xinyuli1204 marked this pull request as draft March 1, 2024 01:40

Xinyu Li and others added 6 commits February 29, 2024 22:36

delete createvector

fe7411c

change ion_graph_multipe

acc6dd5

update id

061372e

node_id

8a3957c

update NodeID

d5717bf

change node.cc

c79ceaf

xinyuli1204 marked this pull request as ready for review March 4, 2024 18:18

xinyuli1204 marked this pull request as draft March 4, 2024 18:50

change graph_multiple

da6d6d1

xinyuli1204 changed the title ~~fix/dup-name-error in graph~~ Fix/dup-name-error in graph Mar 4, 2024

xinyuli1204 marked this pull request as ready for review March 4, 2024 19:12

Xinyu Li and others added 4 commits March 4, 2024 11:16

change ion_graph_create_with_multiple

64df606

update python-binding

ea91ff6

add Graph.py

415f584

delete print

42d5caa

iitaku reviewed Mar 5, 2024

View reviewed changes

replace NodeId to string

4e48349

replace string with NodeId

0ce4b70

iitaku changed the title ~~Fix/dup-name-error in graph~~ Python Graph API / Strongly typed ID Mar 7, 2024

iitaku merged commit 245dc1a into main Mar 7, 2024
3 checks passed

iitaku mentioned this pull request Mar 7, 2024

Graph python binding #246

Closed

xinyuli1204 mentioned this pull request Mar 7, 2024

Duplicated param name error when combine two graphs #251

Closed

xinyuli1204 changed the title ~~Python Graph API / Strongly typed ID~~ Feature/ python graph API and strongly typed ID May 7, 2024

xinyuli1204 deleted the update/python-binding-graph branch November 20, 2024 00:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ python graph API and strongly typed ID #252

Feature/ python graph API and strongly typed ID #252

xinyuli1204 commented Feb 28, 2024 •

edited

Loading

iitaku commented Feb 29, 2024

iitaku Feb 29, 2024

iitaku Feb 29, 2024

xinyuli1204 Mar 4, 2024

xinyuli1204 commented Mar 4, 2024

iitaku Mar 5, 2024

iitaku Mar 5, 2024

iitaku Mar 5, 2024

iitaku Mar 5, 2024

iitaku Mar 5, 2024

iitaku Mar 5, 2024

iitaku Mar 5, 2024

iitaku Mar 5, 2024

iitaku Mar 5, 2024

xinyuli1204 commented Mar 5, 2024 •

edited

Loading

Fixstars-iizuka commented Mar 6, 2024 •

edited

Loading



		template<typename... Args>
		std::vector<Port> create_ports_vector(Args... args) const {

		Impl(const std::string& id_, const std::string& name_, const Halide::Target& target_);
		Impl(const std::string& id_, const std::string& name_, const Halide::Target& target_, const GraphID &graph_id_);

Feature/ python graph API and strongly typed ID #252

Feature/ python graph API and strongly typed ID #252

Conversation

xinyuli1204 commented Feb 28, 2024 • edited Loading

iitaku commented Feb 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinyuli1204 commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinyuli1204 commented Mar 5, 2024 • edited Loading

Fixstars-iizuka commented Mar 6, 2024 • edited Loading

xinyuli1204 commented Feb 28, 2024 •

edited

Loading

xinyuli1204 commented Mar 5, 2024 •

edited

Loading

Fixstars-iizuka commented Mar 6, 2024 •

edited

Loading