Issue 1438 write tar consistency

[marco66colombo]

Description

This PR fixes #1438 by making WriteTar handling consistent and explicit across writers.

Summary

• Move WriteTar if condition one layer higher: check WriterConfig.WriteTar in write_hls() before calling write_tar().
• Keep write_tar() as a pure “write tarball” method (no internal config check).
• Add write_tar support to Catapult backend config (create_initial_config(..., write_tar=False) + WriterConfig['WriteTar']), so Catapult follows the same config path as other backends.
• Extend test_writer_config::test_write_tar coverage to include Catapult.
• Add write_tar support to VivadoAcceleratorBackend.create_initial_config(...) and include VivadoAccelerator in test_writer_config::test_write_tar coverage.

Motivation / Context

Issue #1438 highlights inconsistent artifact behavior and writer semantics.

A key point from prior maintainer discussion is that keeping tar generation disabled by default is better for users, to avoid generating large numbers of extra files during local development and debugging.

This PR keeps that default (WriteTar=False) while making behavior consistent across backends.

Notable behavior note

For Catapult specifically, when WriteTar=True and tar creation is triggered:

• previous behavior: keep existing tar and print “already exists”
• new behavior: overwrite existing tar (consistent with other backends)

This is intentional for consistency across backends.

For VivadoAccelerator, with WriteTar=True, tar can be generated/regenerated twice (once via the Vivado writer path and once via write_new_tar after accelerator-specific files are added). This matches past behavior and is not introduced in this PR.

Follow-up (in progress)

I still need to update tests/CI so artifact generation is always enabled where CI expects tar artifacts.

Possible approaches under consideration:

• Add a central switch in test/pytest/conftest.py (for example wrapping conversion/config creation to force write_tar=True in CI).
• Update each test case explicitly to pass write_tar=True (more explicit, but many more edits required).

I am working on this as a follow-up step.

Type of change

• Bug fix (non-breaking change that fixes an issue)
• Documentation update
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• A new research paper code implementation
• Other (Specify)

Tests

• I ran test/pytest/test_writer_config.py in a local environment.

Test Configuration:

• Local development environment using same image used in CI.

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.

[vloncar]

Why should we move the check outside of the write_tar() function? It was deliberately placed inside so we have a clean set of steps for the writer. Also it is easier to log (we're not doing it now, but could do in the future)

[nghielme]

Because right now if you don't set 'WriteTar' field in the writer config, the behaviour of write_tar() is not the one expected, i.e. it doesn't create the tarball archive. To my understanding, that field this is mainly leveraged for local testing, in which you don't want to have the test folder filled with tarball files.
I think a different and cleaner solution would be to set an environment variable that prevents the tar files to be generated (basically moving back the check inside write_tar(), but checking if the env variable is set this time). This solution has the advantage of being set only once before running the tests and it is applied to all the tests.

[vloncar]

The check is identical, it is just outside of the function now. The idea of the writer is that it follows a set of steps. The tar writer function may or may not write the tar, depending on the config, just like all the other functions write contents based on the provided config. Using env vars to change the behavior of the conversion process while you also have the conversion config itself that controls all other aspects of conversion would be splitting control and introducing inconsistency that will likely be a burden later on.

[marco66colombo]

Thanks, this makes sense. I moved the WriteTar check outside write_tar() following earlier discussion, but I agree that keeping that logic inside write_tar() can preserve the writer-step semantics and original design.
If preferred, I can restore that behavior (move the check back inside write_tar()) while keeping the backend config fixes for Catapult and VivadoAccelerator, plus the related test coverage updates.

[marco66colombo]

As mentioned in the Follow-up section, we still need to decide how to (and if) ensure tar artifact generation in CI.

Should we enable tar artifact generation for every CI test case?

If yes, I see two practical approaches:

1. Central CI toggle in test/pytest/conftest.py

   • Implementation: force write_tar=True for conversions only when a CI toggle is enabled (env var like HLS4ML_FORCE_WRITE_TAR=1 or pytest flag like --force-write-tar).
   • Pros: single-point change, no per-test edits, easy CI on/off.
   • Cons: more implicit behavior (wrapper/monkeypatch), may miss uncommon paths if they bypass wrapped entrypoints.

2. Explicit per-test updates

   • Implementation: update tests to pass write_tar=True in each conversion call.
   • Pros: explicit behavior in each test, finer-grained control.
   • Cons: many edits across the suite, higher maintenance burden.

Happy to proceed with either approach if we want to activate artifact generation.

[marco66colombo]

Since the group was leaning toward using an env var, avoiding edits across many test files, and keeping the default CI behavior as “no tar generation,” I implemented the env-var control in the writer path.

I chose this over a conftest.py approach because forcing tar generation there would require global wrappers/monkeypatching of the methods that create the configs (e.g. convert_from_keras_model), which is more implicit and harder to maintain/debug.

Please let me know if this approach is acceptable, or if you’d prefer the pytest-fixture route instead.

I also restored self.write_tar(...) as an explicit writer step and moved the WriteTar check back inside the tar-writing methods (as suggested in review), so semantics stay consistent.

[nghielme]

I think you need to add the id=[...] here for the proper naming of the tests

[marco66colombo]

thanks for the comment, done

[marco66colombo]

I have one WriterConfig question before I finalize this PR.

Currently, HLSConfig uses backend WriterConfig as-is when present, and only applies defaults when WriterConfig is not there:

hls4ml/hls4ml/model/graph.py

Lines 63 to 71 in b50bd46

	if 'WriterConfig' in self.config:
	self.writer_config = self.config['WriterConfig']
	else:
	self.writer_config = {
	'Namespace': None,
	'WriteWeightsTxt': True,
	'WriteTar': False,
	'TBOutputStream': 'both',
	}

In this PR, Catapult now sets:

 config['WriterConfig'] = {
       'WriteTar': write_tar,
}

That matches existing patterns in some backends (for example):

hls4ml/hls4ml/backends/quartus/quartus_backend.py

Lines 140 to 142 in b50bd46

	config['WriterConfig'] = {
	'WriteTar': write_tar,
	}

hls4ml/hls4ml/backends/oneapi/oneapi_backend.py

Lines 171 to 174 in b50bd46

	config['WriterConfig'] = {
	# TODO: add namespace
	'WriteTar': write_tar,
	}

But other backends provide the full writer-config key set (for example Vivado):

hls4ml/hls4ml/backends/vivado/vivado_backend.py

Lines 282 to 287 in b50bd46

	config['WriterConfig'] = {
	'Namespace': namespace,
	'WriteWeightsTxt': write_weights_txt,
	'WriteTar': write_tar,
	'TBOutputStream': tb_output_stream,
	}

Given this mixed pattern, I was wondering what is the expected policy.

1. Is it acceptable for Catapult to set only WriteTar in WriterConfig?
2. Or should backends that define WriterConfig always provide the full key set for consistency/compatibility?