Latency Pooling Header Updates #973

calad0i · 2024-02-21T05:11:27Z

Precision override moved to Requires #855 now. Expect tests to fail without.

Include and superceeds #917. On the same issue: #947

This PR does the following, for the vivado and vitis backends.

make avg pooling layers in io_parallel mode respect to accum_t.
refactoring codes for pooling layers for io_parallel.
- Performance validated to be equal or better.
- The new implementation avoids a few potential overflowing in tmp and scale in the original avg pool implementation.
  ~~- Avoid overriding global variable names when automatically overriding precisions; derive accum_t properly from impending layer's result_t.~~ Moved to Automatic precision inference #855

With the current setting, pooling layers will be bit-accurate if the impending layer's result_t is configured properly.

Synthesis on some toy models

Type of change

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

Tests

Will be covered in #914 (WIP).

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.

calad0i · 2024-02-21T05:15:47Z

pre-commit.ci autofix

jmitrevs · 2024-02-21T21:45:02Z

hls4ml/backends/vivado/vivado_backend.py

+            # Functional
+            input_layer_name = layer.attributes['inputs'][0]
+        else:
+            # Sequential


I don't fully understand why the if-else is requried. Can't you always query for the previous node?

If we have a functional model that has A->B, A->C, with C being the pooling layer. As the order of B and C is arbitrary, if we query the precious node, we can get B, the sibling of the current node, instead of A.

I don't quite follow. Are the node inputs not correctly filled in some cases?

Not sure for now, but they were not as of Nov 2023. Only for the functional model converted graph, the parent names are filed; for sequential models, the fields were missing.
Could check this again quickly.

So you can use get_input_node or something similar to get the node. https://github.com/fastmachinelearning/hls4ml/blob/main/hls4ml/model/layers.py#L182-L192

Just checked and cannot get the inputs with current hls4ml master.... weird.

Note that it's self.inputs, not self.attributes['inputs'].

(on the Layer object)

Confirmed, modified PR.

jmitrevs · 2024-02-21T21:52:05Z

I wonder if this is part of the inferring of precisions that's in the works: #855. Let me try to update #855 (by making pull requests to it) so we can merge it in soon.

jmitrevs · 2024-04-15T16:42:10Z

There seem to be some real pytest failures associated with this PR. @calad0i, can you take a look?

hls4ml/templates/vivado/nnet_utils/nnet_pooling.h

calad0i · 2024-04-15T20:20:23Z

Fixed vitis and mnist tests. Pytorch test directly crashed, need some debugging to see what is wrong.

jmitrevs · 2024-04-16T16:46:30Z

hls4ml/backends/vivado/vivado_backend.py

-        if isinstance(accum_t.precision, FixedPrecisionType):
-            accum_t.precision.integer += extra_bits
+
+        if pool_op == 'max':


This should not be set here. The user can override it, in which case we should follow what the user requests. Otherwise, if this was set to "auto", then the automatic precision inference (#855) will do something similar. We can check if it does the right thing there and fix it if needed (but let me check that my updates that went in that PR first before adding code to it.)

This part is directly modified following the original implementation; I didn't move things around.

We can move this to #855 _infer_pooling_precision, but please let me know how to apply the changes as I cannot commit there. The current implementation in #855 is probably faulty.

This is the current version that I have: https://github.com/jmitrevs/hls4ml/blob/qonnx-1p0/hls4ml/model/optimizer/passes/infer_precision.py. I will try to extract it and make a PR to 855 with it within the next 2 hours or so.

I made a PR to Vladimir's branch: vloncar#55 . You can probably do the same. I don't think I explicitly did anything for pooling, but there is the merge Average and Add that probably have the same logic in my PR.

Opened PR at #56. This PR will be updated if that one got merged.

jmitrevs · 2024-04-18T15:22:26Z

hls4ml/backends/vivado/vivado_backend.py

@@ -376,37 +373,14 @@ def init_depconv2d(self, layer):
        )  # TODO Once we have SeparableConv implementation for io_parallel this should be set properly
        layer.set_attr('implementation', layer.model.config.get_conv_implementation(layer).lower())

-    def _set_pooling_accum_t(self, layer, pool_size):


We shouldn't merge this change before we have the subsequent change in auto precision inference. Right now it's removing features that already exist (and catapult customers may be using) without replacing it right away. For Vitis and Vivado it is fine to not add something that doesn't yet exist, but one has to be more careful about removing already existing features. If we want to decouple things, we should move the catapult update to a separate PR.

The current _set_pooling_accum_t is flawed (overrides default_t in most cases, doesn't always fix overflow as well; multiple pooling layers will results in a huge default model bitwidth thanks to the += operation), and I fixed it in the last iteration of this PR.

This PR will not work without proper accum_t set, and that's why I fixed it right here in the last iteration. As you suggested, I integrated it with #855, and I have marked #855 as required by this PR.

I still think it's problematic for us to remove this from the Catapult backend. We can make a separate PR just removing it and ask them to comment and review it. That's why I was not wanting the other stuff to be dependent on the Seimens signoff. It's a user interface change for their users, so they have to be on board and scheduling the change.

Generally, unless they are code maintenance-type changes or purely adding new features, Catapult backend changes need Siemens approval.

How about I just revert the last commit and let #855 remove this override for vivado, so that this PR is self-contained?
Or if #855 is planned to be merged soon, I will just revert for the catapult backend, though the part in question is probably copy-pasta from the current Vivado backends.

We are hoping to merge #855 soon. The last changes failed the pytests, so after chaging "ave" to "average" we are trying again. Vivado should not have the overrides since it never had them. It should start with the new way of doing things from the beginning. (Actually, I was wrong about Vivado not having had the overrides before.)

I would probably make the Catapult change a separate PR, though. We do eventually want that.

As #855 does not remove that override, I will remove the Vivado override and leave the Siemens one as-is.

I hadn't appreciated that there was already some auto precision setting for Vivado. Nevertheless, I believe removing it is the right thing to do since we have the auto precision. I will announce it.

latency pooling overhaul vivado latency pooling overhaul vitis latency pooling overhaul, fix comment fix boundry cond fix syn issues latency pooling overhaul Fix pooling accum_t autoset & avoid global override [pre-commit.ci] auto fixes from pre-commit hooks better way to get inp layer name fix for vitis / input_t fetch torch padding fix avoid name dup in torch api test rm pooling precision override in favor of fastmachinelearning#855

calad0i · 2024-04-26T22:05:00Z

test/pytest/test_weight_writer.py is not updated in this PR though...
pre-commit.ci autofix

jmitrevs · 2024-04-26T22:06:33Z

Don't worry about the test_weight_writer.py

jmitrevs · 2024-04-26T22:07:43Z

I am not sure if our pytest environment is functional. I can try running them again, but they may fail for infrastructure reasons

jmitrevs · 2024-05-03T23:40:34Z

The test_qkeras.py::test_qgru[Vivado] failure we can ignore. For cnn_mnist, why does this error cause the failure? We mentioned about possibly removing that test, but I want to understand why it's failing. Would setting the accumulator to auto fix the issue?

calad0i · 2024-05-04T01:28:58Z

The test_qkeras.py::test_qgru[Vivado] failure we can ignore. For cnn_mnist, why does this error cause the failure? We mentioned about possibly removing that test, but I want to understand why it's failing. Would setting the accumulator to auto fix the issue?

Correct. The reason to fail is exactly overflow in the avg pool, which had an overriding mechanism but is now removed.
Adding a line hls_config['LayerName']['avg_pool']['Precision']['accum'] = 'auto' in the test and name the layer properly will fix it.

jmitrevs · 2024-05-16T16:03:54Z

Let's add the fix to the test and maybe merge this? I think making auto a default for some layer types could be a different PR, and I think it's really a change in the config_from_keras_model functionality.

jmitrevs · 2024-05-31T14:56:30Z

We can fix the failing test_cnn_mnist.py with:

(fastml310) jovans-mac:pytest jmitrevs$ git diff
diff --git a/test/pytest/test_cnn_mnist.py b/test/pytest/test_cnn_mnist.py
index ab3365f2..a8b77862 100644
--- a/test/pytest/test_cnn_mnist.py
+++ b/test/pytest/test_cnn_mnist.py
@@ -70,6 +70,8 @@ def test_mnist_cnn(keras_model, mnist_data, backend, io_type, strategy):
     hls_config = hls4ml.utils.config_from_keras_model(keras_model, granularity='name', backend=backend)
     hls_config['Model']['Strategy'] = strategy
     hls_config['LayerName']['softmax']['Implementation'] = 'stable'
+    hls_config['LayerName']['average_pooling2d']['Precision']['accum'] = 'auto'
+    hls_config['LayerName']['max_pooling2d']['Precision']['accum'] = 'auto'
     output_dir = str(test_root_path / f'hls4mlprj_cnn_mnist_{backend}_{io_type}_{strategy}')
 
     hls_model = hls4ml.converters.convert_from_keras_model(

jmitrevs reviewed Feb 21, 2024

View reviewed changes

calad0i mentioned this pull request Feb 21, 2024

vitis backend tarball fix #972

Merged

7 tasks

calad0i mentioned this pull request Apr 5, 2024

[DRAFT] Fix pooling accum_t autoset & avoid global override #917

Closed

7 tasks

jmitrevs added the please test Trigger testing by creating local PR branch label Apr 15, 2024

jmitrevs reviewed Apr 15, 2024

View reviewed changes

hls4ml/templates/vivado/nnet_utils/nnet_pooling.h Show resolved Hide resolved

jmitrevs mentioned this pull request Apr 15, 2024

Generalize pooling accumulator inference #947

Closed

8 tasks

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

jmitrevs reviewed Apr 16, 2024

View reviewed changes

calad0i mentioned this pull request Apr 17, 2024

fix pooling precision inference vloncar/hls4ml#56

Merged

jmitrevs reviewed Apr 18, 2024

View reviewed changes

calad0i added 2 commits April 24, 2024 18:08

Keep pooling global override for Siemens

0ab455e

calad0i force-pushed the latency_pool branch from d96a140 to 0ab455e Compare April 26, 2024 19:42

calad0i mentioned this pull request Apr 26, 2024

Add support for HGQ proxy model #914

Open

7 tasks

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

update weight writer test

307a128

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

Merge branch 'main' into latency_pool

57c1920

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

Merge branch 'main' into latency_pool

cfa2d1b

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

fix mnist test

c55f17d

Latency Pooling Header Updates #973

Are you sure you want to change the base?

Latency Pooling Header Updates #973

Conversation

calad0i commented Feb 21, 2024 • edited

Precision override moved to Requires #855 now. Expect tests to fail without.

Type of change

Tests

Checklist

calad0i commented Feb 21, 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

Choose a reason for hiding this comment

jmitrevs commented Feb 21, 2024

jmitrevs commented Apr 15, 2024 • edited

calad0i commented Apr 15, 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

Choose a reason for hiding this comment

jmitrevs Apr 18, 2024 • edited

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

calad0i commented Apr 26, 2024

jmitrevs commented Apr 26, 2024

jmitrevs commented Apr 26, 2024

jmitrevs commented May 3, 2024

calad0i commented May 4, 2024

jmitrevs commented May 16, 2024

jmitrevs commented May 31, 2024

calad0i commented Feb 21, 2024 •

edited

jmitrevs commented Apr 15, 2024 •

edited

jmitrevs Apr 18, 2024 •

edited