Multi output fix 2 #1103

calad0i · 2024-10-29T23:42:22Z

Description

Fix a couple of corner cases where model generation/compile fails. Now one can let a model output at (almost) all layers (except flatten/reshape simultaneously with its input in io_parallel).

Fix output node updating mechanism. The current parser assumes set(outputs)-set(inputs) are the output nodes, which is not true if some intermediate nodes are being outputted, or if any inputs is unused.
remove_node is updated to correctly rewire in all cases, including leaf/intermediate+leaf nodes. Deprecate arg rewire.
Add early stream flatten optimizer to pass original size to potential repack optimizers
Added 1-to-3 repack catapult template

Type of change

Bug fix (non-breaking change that fixes an issue)

Tests

test/pytest/test_multiout_network.py/test_multi_output_nn_2

Checklist

I have read the guidelines for contributing.
I have commented my code, particularly in hard-to-understand areas.
[] I have made corresponding changes to the documentation.
My changes generate no new warnings.
I have installed and run pre-commit on the files I edited or added.
I have added tests that prove my fix is effective or that my feature works.

_

…nto multi_output_fix_2

hls4ml/backends/fpga/passes/clone.py

test/pytest/test_multiout_network.py

hls4ml/model/graph.py

jmitrevs · 2024-11-09T02:26:32Z

Note, this PR includes #1119.

hls4ml/model/graph.py

Shouldn't have yet approved while we discuss the comments.

jmitrevs · 2024-11-09T03:06:53Z

This looks good to me, and I would approve it, but didn't want to skip the step of looking at the above comments.

vloncar

Looks very good so far, I'd just recommend minor cosmetic changes

vloncar · 2024-11-09T18:29:45Z

hls4ml/backends/fpga/passes/clone.py

+            if n_outputs == 1:
+                continue
+            if n_outputs > 3:
+                msg = f'ERROR: Cloning output {output} of {node.__class__.__name__} ({node.name}) more than 3 times not currently supported'  # noqa: E501


This will print the node class (a backend-specific instance) which may be confusing to the user. We have a node.class_name for this purpose.

vloncar · 2024-11-09T18:29:48Z

hls4ml/backends/fpga/passes/clone.py

+                raise ValueError(msg)
+
+            out_var = node.get_output_variable(output)
+            attrs = {'size': np.prod(out_var.shape)}


The output of np.prod() will be an ndarray, which works for now, but may trip up in the future (serialization and attribute validation will fail, since we would expect an integral value). Since we're already changing the code here, we might as well sort it out now.

vloncar · 2024-11-09T18:31:44Z

hls4ml/backends/fpga/passes/inplace_parallel_reshape.py

@@ -11,14 +11,19 @@ class InplaceParallelReshape(OptimizerPass):
    """

    def match(self, node):
-        return isinstance(node, Reshape)
+        if not isinstance(node, Reshape):
+            return


Can we rewrite this so it returns booleans or perhaps combine the two checks?

vloncar · 2024-11-09T18:35:08Z

hls4ml/backends/fpga/passes/inplace_parallel_reshape.py

+            prev_node = node.get_input_node()
+            assert (
+                prev_node.name not in model.outputs
+            ), f"Cannot output node {prev_node.name}: reshape is a no-op in io_parallel. As a result, the previous node {prev_node.name}'s output will be used as the output. However, this node is already an output."  # noqa: E501


Does this trigger the bug/feature of black with formatting asserts, or is E501 just out of laziness? 😄

And a nitpick: We prefer single quotes wherever possible

vloncar · 2024-11-09T18:35:57Z

hls4ml/backends/fpga/passes/inplace_stream_flatten.py

            return False
+        io_type = node.model.config.get_config_value('IOType')


why not a one-liner? 😉

vloncar · 2024-11-09T18:38:30Z

hls4ml/model/graph.py

-            rewire (bool, optional): If `True`, connects the outputs of the previous node
-                to the inputs of the next node
+            node (Layer): The node to remove rewire (bool, optional):
+            Deprecated, no effect


Would marking this as deprecated according to the docs work? Would be nice to see

vloncar · 2024-11-09T18:44:01Z

test/pytest/test_multiout_network.py

+@pytest.mark.parametrize('backend', ['Vivado', 'Quartus', 'Vitis', 'Catapult', 'OneAPI'])
+@pytest.mark.parametrize('io_type', ['io_parallel', 'io_stream'])
+@pytest.mark.parametrize('strategy', ['latency', 'resource'])
+def test_multi_output_nn_2(model2, data2, backend: str, io_type: str, strategy: str):


Can we be more descriptive instead of appending 2 to model, data and test?

hls4ml/model/graph.py

calad0i added the please test Trigger testing by creating local PR branch label Oct 29, 2024

calad0i force-pushed the multi_output_fix_2 branch from b715dc2 to 2cdedb6 Compare October 30, 2024 00:10

calad0i added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Oct 30, 2024

jmitrevs added this to the v1.0.0 milestone Nov 7, 2024

jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Nov 7, 2024

calad0i added 8 commits November 8, 2024 18:09

multi output and flatten@streaming fix

ea0778a

relaxing remove node shape check cond

a8239c2

fix regression errors

e352c6c

catapult and oneapi tests

783c1f4

add catapult 3-clone

a8b9223

rm ill-condition

e0b59a9

_

allow removing nodes w/o i or o

c34de9e

Merge remote-tracking branch 'upstream/make_Constant_without_input' i…

0c2e90e

…nto multi_output_fix_2

calad0i force-pushed the multi_output_fix_2 branch from 8ce2a28 to 0c2e90e Compare November 8, 2024 18:32

calad0i added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Nov 8, 2024

jmitrevs reviewed Nov 8, 2024

View reviewed changes

hls4ml/backends/fpga/passes/clone.py Outdated Show resolved Hide resolved

jmitrevs reviewed Nov 8, 2024

View reviewed changes

test/pytest/test_multiout_network.py Show resolved Hide resolved

jmitrevs reviewed Nov 9, 2024

View reviewed changes

hls4ml/model/graph.py Show resolved Hide resolved

jmitrevs reviewed Nov 9, 2024

View reviewed changes

hls4ml/model/graph.py Outdated Show resolved Hide resolved

jmitrevs previously approved these changes Nov 9, 2024

View reviewed changes

jmitrevs self-requested a review November 9, 2024 03:03

chore

7df1e0d

calad0i enabled auto-merge (rebase) November 9, 2024 03:33

calad0i removed the please test Trigger testing by creating local PR branch label Nov 9, 2024

calad0i added the please test Trigger testing by creating local PR branch label Nov 9, 2024

vloncar requested changes Nov 9, 2024

View reviewed changes

calad0i force-pushed the multi_output_fix_2 branch from e63225a to d2d907b Compare November 10, 2024 21:51

cosmatic

e5023c8

calad0i force-pushed the multi_output_fix_2 branch from d2d907b to e5023c8 Compare November 10, 2024 21:52

jmitrevs reviewed Nov 11, 2024

View reviewed changes

hls4ml/model/graph.py Outdated Show resolved Hide resolved

typo and docstring

bc657e1

calad0i added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Nov 13, 2024

allow io_stream if used as model output

45f96f6

calad0i force-pushed the multi_output_fix_2 branch from b1e7349 to 45f96f6 Compare November 13, 2024 06:06

calad0i added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Nov 13, 2024

Merge branch 'main' into multi_output_fix_2

f9ccbef

JanFSchulte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Nov 13, 2024

calad0i mentioned this pull request Nov 15, 2024

⚡ Add ESD Support to hls4ml #1129

Closed

2 tasks

vloncar approved these changes Nov 15, 2024

View reviewed changes

calad0i merged commit ef2e8f4 into fastmachinelearning:main Nov 15, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi output fix 2 #1103

Multi output fix 2 #1103

calad0i commented Oct 29, 2024

jmitrevs commented Nov 9, 2024

jmitrevs commented Nov 9, 2024

vloncar left a comment

vloncar Nov 9, 2024

vloncar Nov 9, 2024

vloncar Nov 9, 2024

vloncar Nov 9, 2024

vloncar Nov 9, 2024

vloncar Nov 9, 2024

vloncar Nov 9, 2024

		return False
		io_type = node.model.config.get_config_value('IOType')

Multi output fix 2 #1103

Multi output fix 2 #1103

Conversation

calad0i commented Oct 29, 2024

Description

Type of change

Tests

Checklist

jmitrevs commented Nov 9, 2024

jmitrevs commented Nov 9, 2024

vloncar left a comment

Choose a reason for hiding this comment

vloncar Nov 9, 2024

Choose a reason for hiding this comment

vloncar Nov 9, 2024

Choose a reason for hiding this comment

vloncar Nov 9, 2024

Choose a reason for hiding this comment

vloncar Nov 9, 2024

Choose a reason for hiding this comment

vloncar Nov 9, 2024

Choose a reason for hiding this comment

vloncar Nov 9, 2024

Choose a reason for hiding this comment

vloncar Nov 9, 2024

Choose a reason for hiding this comment