Skip to content

Bidirectional RNN layer support for Keras frontend and Vitis backend #1310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 38 commits into from
Jul 23, 2025

Conversation

enlupi
Copy link
Contributor

@enlupi enlupi commented Jun 12, 2025

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.

@enlupi enlupi force-pushed the vivado_bidir_general branch from f929985 to 1c16616 Compare June 12, 2025 15:00
@enlupi enlupi marked this pull request as ready for review June 23, 2025 12:16
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.'
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

temp_layer = rnn_forward_layer.copy()
rnn_forward_layer = rnn_backward_layer.copy()
rnn_backward_layer = temp_layer
swapped_order = True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -11,13 +11,15 @@
)

rnn_layers = ['SimpleRNN', 'LSTM', 'GRU']
merge_modes = ['sum', 'mul', 'concat', 'ave']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

h_state = h_state_forward;
s_state = s_state_forward;
}
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove commented code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the comments, thank you.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these couts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed them, thank you.

else {
h_state = h_state_forward;
}
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove commented code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the comments, thank you.

@JanFSchulte
Copy link
Contributor

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
Copy link

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'

@JanFSchulte JanFSchulte added the please test Trigger testing by creating local PR branch label Jun 27, 2025
@enlupi
Copy link
Contributor Author

enlupi commented Jul 22, 2025

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 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
Copy link
Contributor

Test failures unrelated, this is ready for merge.

@JanFSchulte JanFSchulte merged commit abcf95c into fastmachinelearning:main Jul 23, 2025
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants