Bidirectional RNN layer support for Keras frontend and Vitis backend by enlupi · Pull Request #1310 · fastmachinelearning/hls4ml

enlupi · 2025-06-12T14:12:23Z

Description

This PR adds support for Bidirectional RNN layers using Keras V2 and V3 with the Vitis backend in io_parallel mode. The forward and backward layer can be either LSTM or GRU, and their architecture independent one from the other.

It also fixes an issue when using recurrent layers (SimpleRNN, LSTM and GRU) with Keras V3. Previously, an extra activation layer was automatically added after the mentioned layers: this produced wrong predictions, as the activation is already internal to the layers.

Type of change

Bug fix
New feature

Tests

Unit test in test/pytest/test_rnn.py was updated to also check parsing and accuracy for a Bidirectional layer.

Test Configuration:

The new tests are carried out using only Vivado or Vitis backend and io_parallel mode.

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.

…ctures

JanFSchulte · 2025-06-26T19:20:50Z

+        print(
+            f'WARNING: The selected order for forward and backward layers in "{node.name}" ({node.class_name}) is not '
+            'supported in Vitis backend. Switching to forward layer first, backward layer last.'
+        )


Where does this switching actually happen? Or is this meant to prompt the user to do it themselves? Also, this probably should just be caught directly in the parser where the swapped_order attribute is determined.

The switch happens during the parsing, more precisely in line 125 of recurrent.py.
i moved the warning comment directly in the parser as suggested.

JanFSchulte · 2025-06-26T19:24:39Z

+            f'WARNING: "{merge_mode}" merge mode in "{node.name}" ({node.class_name}) is not supported in Vitis backend. '
+            'Switching to "concat" merge mode.'
+        )
+        node.set_attr('merge_mode', 'concat')


Why are we doing this here instead of just doing it during the parsing in the converter?

Because in the future there could be other backends that do implement different merge modes, while Vitis remains lacking. It is not generally impossible to implement.

JanFSchulte · 2025-06-26T19:33:42Z

+        if params['pass_initial_states'] == 'true':
+            params['input2_t'] = node.get_input_variable(node.inputs[1]).type.name
+            params['input2'] = node.get_input_variable(node.inputs[1]).name
+            if node.class_name == 'BLSTM':


Should this be just LSTM? I don't see BLSTM as a class name anywhere else.

This was an outdated code snippet. It has been removed.

JanFSchulte · 2025-06-26T19:36:33Z

+            temp_layer = rnn_forward_layer.copy()
+            rnn_forward_layer = rnn_backward_layer.copy()
+            rnn_backward_layer = temp_layer
+            swapped_order = True


I don't think this case is supported, right? We should probably just throw an exception here and tell the user.

At the moment we swap the order of the layers, throw a warning and proceed (please see also the first comment in this chain). Do you think it would be best to throw an exception instead?

I think this is probably fine, thanks for the explanation.

JanFSchulte · 2025-06-26T19:40:54Z

 )

 rnn_layers = ['SimpleRNN', 'LSTM', 'GRU']
+merge_modes = ['sum', 'mul', 'concat', 'ave']


Why list the other 3 here when only concat is supported?

This was done because concat is the only one currently supported, but I wanted the parser to be more general. In any case, this check is also carried out internally by Keras when creating the layer, so I removed it to avoid redundancy.

JanFSchulte · 2025-06-26T19:44:26Z

+        h_state = h_state_forward;
+        s_state = s_state_forward;
+    }
+*/


Please remove commented code.

Removed the comments, thank you.

JanFSchulte · 2025-06-26T19:44:48Z

+    std::cout << "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~" << std::endl << std::endl;
+    std::cout << "Data_t size: " << data_T::size << std::endl;
+    std::cout << std::endl << "~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~" << std::endl << std::endl;
+


Please remove these couts.

Removed them, thank you.

JanFSchulte · 2025-06-26T19:45:06Z

+    else {
+        h_state = h_state_forward;
+    }
+*/


Please remove commented code.

Removed the comments, thank you.

JanFSchulte · 2025-06-26T19:52:20Z

Generally this looks good to me, comments are minor. I'll wait until some things are merged that should fix some tests failures and then run the CI.

rimalroc · 2025-06-27T10:01:06Z

Hi, thank you for implementing this, have you tried this with kerasv3? the mentioned test unit is using keras2 only
It seems to fall to the keras v2 handler, but I get the following error.

v2 handler used for layer bidirectional
Traceback (most recent call last):
  File "/work/NGT/ngt2.2-toy-simulation/./convert/test_convert.py", line 180, in <module>
    hls_model = converttools.conv_to_hls(models[mod_id], model,REWRITE_CONF=args.rewriteconf, verbose=True)
  File "/work/NGT/ngt2.2-toy-simulation/convert/../convert/converttools.py", line 211, in conv_to_hls
    hls_model = hls4ml.converters.convert_from_keras_model(
  File "/work/NGT/hls4ml_enlupi/hls4ml/hls4ml/utils/dependency.py", line 46, in inner
    return f(*args, **kwargs)
  File "/work/NGT/hls4ml_enlupi/hls4ml/hls4ml/converters/__init__.py", line 223, in convert_from_keras_model
    return keras_v3_to_hls(config)
  File "/work/NGT/hls4ml_enlupi/hls4ml/hls4ml/converters/keras_v3_to_hls.py", line 294, in keras_v3_to_hls
    return ModelGraph.from_layer_list(config, layer_list, input_layers, output_layers)
  File "/work/NGT/hls4ml_enlupi/hls4ml/hls4ml/model/graph.py", line 443, in from_layer_list
    model._make_graph(layer_list)
  File "/work/NGT/hls4ml_enlupi/hls4ml/hls4ml/model/graph.py", line 477, in _make_graph
    self.graph[name] = self.make_node(kind, name, layer, inputs, outputs)
  File "/work/NGT/hls4ml_enlupi/hls4ml/hls4ml/model/graph.py", line 566, in make_node
    node = layer_cls(self, name, attributes, inputs, outputs, initialize)
  File "/work/NGT/hls4ml_enlupi/hls4ml/hls4ml/model/layers.py", line 122, in __init__
    self.initialize()
  File "/work/NGT/hls4ml_enlupi/hls4ml/hls4ml/model/layers.py", line 1530, in initialize
    self.add_weights_variable(name=f'{dir}_weight', var_name=(f'w_{dir[0]}_' + '{index}'))
  File "/work/NGT/hls4ml_enlupi/hls4ml/hls4ml/model/layers.py", line 337, in add_weights_variable
    var = WeightVariable(
  File "/work/NGT/hls4ml_enlupi/hls4ml/hls4ml/model/types.py", line 562, in __init__
    self.shape = list(self.data.shape)
AttributeError: 'NoneType' object has no attribute 'shape'

enlupi · 2025-07-22T13:18:08Z

I now added support for Keras V3, creating a custom parser for the Bidirectional layer and fixing some unintended behavior when calling the v2 handlers for the LSTM and GRU layers.
Now the test unit works fine for me both with keras v2 and v3. Please let me know if you still experience some issues.

JanFSchulte · 2025-07-23T13:27:55Z

Test failures unrelated, this is ready for merge.

Enrico Lupi and others added 22 commits June 12, 2025 16:35

ADD parsing for bidirectional RNN layers

f975066

Implement bidirectional rnn layers

8121281

ADD fixes

fc1e950

FIX resource strategy

f47bb5a

FIX infer precision for bidirectional rnn

c469793

FIX eliminate activation after bidirectional rnn

4c3d26e

FIX bidirectional layers name

5eef679

ADD tests for bidirectional layer

a9546c7

FIX weight name and ADD backward layer architecture check

0246dae

FIX static and non-static Bidirectional layers

7428af7

ADD parse general bidirectional layer with possibly different archite…

d2c6cc0

…ctures

ADD paring for general bidirectional layer

edf7cdf

ADD gnerale bidirectional wrapper

d882310

ADD Bidirectional layers support

4ed22c9

ADD support for reverse order layers

dd4f220

ADD feature check for merge mode and layers order

de803b7

ADD io type feature check

070fdc2

FIX n_out in case of merge_mode != concat

b65c730

ADD pytest for Bidirectional layer

b55cd04

FIX posible directions for LSTM and GRU

e8fae54

FIX spelling mistake

a1500e4

FIX order

1c16616

enlupi force-pushed the vivado_bidir_general branch from f929985 to 1c16616 Compare June 12, 2025 15:00

Enrico Lupi and others added 3 commits June 12, 2025 17:10

FIX remove unused import

2fc981c

FIX blank space

48f4fe2

Merge branch 'main' into vivado_bidir_general

64ab715

enlupi marked this pull request as ready for review June 23, 2025 12:16

JanFSchulte reviewed Jun 26, 2025

View reviewed changes

Merge branch 'main' into vivado_bidir_general

1b919c8

JanFSchulte added the please test Trigger testing by creating local PR branch label Jun 27, 2025

enlupi and others added 8 commits July 14, 2025 16:36

Merge branch 'fastmachinelearning:main' into vivado_bidir_general

20ff35e

RM old comments

734d42f

MV check for out-of-order layers from passes to parsing

7c128f7

RM check for bidirectional layers order (now done during parsing)

689ef0f

ADD recurrent layers support for keras V3

188a111

change code order

c1299c3

FIX test for keras v3

d5b030f

FIX keras v3 support for bidirectional layers

2df11e3

Enrico Lupi and others added 3 commits July 22, 2025 15:20

FIX add back all test cases

47d27e7

Merge branch 'main' into vivado_bidir_general

a4eb17d

FIX import keras

225f427

JanFSchulte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jul 22, 2025

JanFSchulte approved these changes Jul 23, 2025

View reviewed changes

Merge branch 'main' into vivado_bidir_general

97cc416

JanFSchulte merged commit abcf95c into fastmachinelearning:main Jul 23, 2025
3 of 5 checks passed

enlupi mentioned this pull request Jul 24, 2025

ADD parsing class for rnn layers in keras V3 #1345

Merged

7 tasks

JanFSchulte mentioned this pull request Aug 4, 2025

Fix missing return statement in time distributed parser #1353

Merged

1 task

Uh oh!

Conversation

enlupi commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Tests

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanFSchulte commented Jun 26, 2025

Uh oh!

rimalroc commented Jun 27, 2025

Uh oh!

enlupi commented Jul 22, 2025

Uh oh!

JanFSchulte commented Jul 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

enlupi commented Jun 12, 2025 •

edited

Loading