Repository navigation
Remove RecurrentReuseFactor from the Altera backend - #1581
JanFSchulte merged 4 commits into
Conversation
On Altera, the SimpleRNN, LSTM and GRU initializers overwrote the user's RecurrentReuseFactor with the layer's ReuseFactor, so the setting that config_from_keras_model wrote into every config had no effect. Stop registering the setting, matching what fastmachinelearning#1577 does for Vivado and Vitis, and warn when a config still sets it. The internal recurrent_reuse_factor attribute is still set from ReuseFactor. Fixes fastmachinelearning#1580
JanFSchulte
left a comment
There was a problem hiding this comment.
I have one relevant comment and one nitpick.
| if layer.attributes['n_in'] is None: | ||
| raise Exception('Input length of Embedding layer must be specified.') | ||
|
|
||
| def _warn_removed_reuse_factors(self, layer, attributes): |
There was a problem hiding this comment.
This duplicates a function that #1577 adds to the vivado backend. Both should be moved into the generic FPGABackend class.
| if attribute in layer.attributes: | ||
| warn( | ||
| f'{convert_to_pascal_case(attribute)} of layer "{layer.name}" ({layer.class_name}) is no longer used. ' | ||
| 'The ReuseFactor of the layer applies to all of its multiplications.' |
There was a problem hiding this comment.
This is very slightly inaccurate since the reuse value for the LSTM and GRU recurrent multiplications is set to the closest valid value to the ReuseFactor of the layer. Not sure if we need to specify that in the warning for clarity @vloncar?
There was a problem hiding this comment.
Thanks. I left the wording as is for now; happy to mention the closest-valid adjustment for LSTM/GRU if @vloncar thinks it's worth spelling out.
…urrent-reuse-factor
The Vivado and Altera backends had identical copies of the helper; keep a single one in the shared FPGABackend class.
|
@copilot resolve the merge conflicts in this pull request |
…urrent-reuse-factor # Conflicts: # docs/ir/attributes.rst # hls4ml/backends/vivado/vivado_backend.py
|
@JanFSchulte Copilot can't push to my fork, so I merged main myself in 897aa3b. The conflicts were in |
Description
On the Altera backend,
RecurrentReuseFactorof SimpleRNN, LSTM and GRU layers had no effect._register_layer_attributesregistered it as aConfigurableAttribute, soconfig_from_keras_model(..., backend='Altera')wrote it into every generated config, butinit_gru,init_lstmandinit_simple_rnneach began withlayer.set_attr('recurrent_reuse_factor', reuse_factor), which replaced the user's value with the layer'sReuseFactor.This follows the recommended option in the issue, matching what #1577 does for Vivado and Vitis:
recurrent_reuse_factorConfigurableAttributeis no longer registered for SimpleRNN, LSTM and GRU on Altera, so generated configs no longer containRecurrentReuseFactor. Thetable_sizeandtableregistrations in the same loop are unchanged.recurrent_reuse_factorattribute is still set fromReuseFactorin the three initializers, so the GRU template andset_closest_reuse_factorkeep working as before.RecurrentReuseFactornow gets a warning.Layer.__init__copies every config key intolayer.attributes, registered or not, so the key is only present if the user's config set it. The check runs at the start of each initializer, before the internal value is written._warn_removed_reuse_factorsis copied from Fix recurrent layers on Vivado and Vitis, and add SimpleRNN #1577's Vivado backend, since that PR isn't merged yet and the Altera backend doesn't inherit from the Vivado one.docs/ir/attributes.rst: Altera is removed from therecurrent_reuse_factor"Available in" lines for SimpleRNN, LSTM and GRU. Runningdocs/attr_doc_gen.pyon currentmainalso addsVitisUnifiedto every "Available in" line. That has nothing to do with this change and Fix recurrent layers on Vivado and Vitis, and add SimpleRNN #1577 mentions a separate docs PR that regenerates the whole file, so only the three relevant hunks of the generated output are kept here.Vivado, Vitis, Catapult and Quartus are not touched. Catapult has the same problem, but as the issue notes, converting an LSTM with Catapult currently fails for an unrelated reason.
Fixes #1580
Type of change
io_type, Strategy, or config attributeAffected areas
Backends:
hls4ml.model)Frontends:
Components:
hls4ml/templates/Configurations affected and exercised
io_typeNew configuration axis introduced by this PR (if any): none
Impact on generated HLS
Numerical behaviour: bit-exact vs main. The value used for the recurrent multiplication was already
ReuseFactor; only the config surface and the warning change. No synthesis was run.Tests
Added
test_altera_recurrent_reuse_factor_removedtotest/pytest/test_rnn.py, parametrized over SimpleRNN, LSTM and GRU. It checks thatconfig_from_keras_model(..., backend='Altera')no longer writesRecurrentReuseFactor, that setting it (ReuseFactor8,RecurrentReuseFactor4) raises aUserWarningand the layer'srecurrent_reuse_factoris 8, and that no warning is raised when the key isn't in the config. It only converts, so no HLS tools are needed. All three cases fail onmainand pass with the change.The existing Altera cases in
test_rnn.py(-k Altera) give the same 16 failures before and after the change, all atcompile()withCould not find ahls, because the Altera HLS compiler isn't installed here.test_rnn.py -k parsingpasses (8).test_recurrent_pytorch.pywas not run (no PyTorch in this environment).Test configuration (OS, Python, ML framework version, HLS tool version): Linux, Python 3.12, Keras 3.15.1 with the JAX 0.11.2 backend, NumPy 2.5.4; no HLS tools.
AI assistance disclosure
Tool(s) and model(s): Cursor agent CLI (grok-4.7-xhigh)
Where it was used: the whole change (backend edit, test, docs hunk); the test and the scope were trimmed afterwards.
If anything other than None is ticked, confirm all of the following:
it. I have read and understood every line and can explain and defend it
in review.
no invented APIs, config keys, pragmas or citations.
actually performed, not from agent output.
my knowledge it does not reproduce third-party code that would conflict
with it.
Checklist
Required:
pre-commiton the files I edited.test/pytestcovering this change (a bug fix shouldadd a test that fails on
mainand passes here).commented-out blocks or unrelated changes.
If applicable:
docs/updated.Release note