Skip to content
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

Add namespaces and optional writer config #986

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vloncar
Copy link
Contributor

@vloncar vloncar commented Mar 28, 2024

Description

As observed in the emulator in CMSSW, there are some issues with global namespace being polluted when multiple versions of the same model are loaded. The accepted solution was to use namespaces. This PR follows up in hls4ml by adding the ability to generate namespace and optionally not write .txt files (also requested by the emulator people) and not to write .tar.gz (everyone hates this). The new configurable options are placed under WriterConfig as it made no sense to shove them under HLSConfig. In the future with the new config scheme we will re-evaluate this.

Type of change

  • Documentation update
  • New feature (non-breaking change which adds functionality)

Tests

There's a test in test_writer_config.py for the 3 new config options

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 vloncar added the please test Trigger testing by creating local PR branch label Mar 28, 2024
@vloncar vloncar added this to the v1.0.0 milestone Mar 28, 2024
@vloncar vloncar requested a review from jmduarte March 28, 2024 20:51
@jmduarte jmduarte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Apr 8, 2024
@jmduarte
Copy link
Member

jmduarte commented Apr 8, 2024

After discussing this with @thesps @aloeliger, we realized that the definition of nnet_utils may also collide (to be checked), which we want to prevent in principle due to potential differences in hls4ml versions, etc., so I think we should also try to protect those in this PR

@thesps
Copy link
Contributor

thesps commented Apr 8, 2024

Could we also protect the nnet_utils function definitions? They are currently all in the namespace nnet, but we want to allow that each model/project can use a different hls4ml version/implementation, so it would be good to change that to the same namespace that applies to the weights.

@vloncar
Copy link
Contributor Author

vloncar commented Apr 9, 2024

It would be better if we worked on understanding the root of the problem rather than patching up in anticipation of unexpected behavior. Making the nnet_utils part of the namespace won't solve the issue if function loading is the problem, as the interface fundamentally depends on the existence of create_model and destroy_model functions in every loaded library and you can't put those in namesepaces. We have not seen this failing which is expected behavior according to the standard. We have also not seen bad casts which suggests types don't clash. We have only seen this in initialization of static arrays. I was unable to reproduce this and create a MWE, making me think CMSSW environment is more specific and we should understand what goes on there better. Or we just forget about all of this emulator business and go back to the solution of pasting the code of all models in CMSSW codebase if we want to be safe.

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.

None yet

3 participants