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

Decouple pipeline style from strategy #781

Merged
merged 3 commits into from
May 14, 2023

Conversation

vloncar
Copy link
Contributor

@vloncar vloncar commented May 3, 2023

Description

We control the top function pipeline style with the strategy, and while this works, sometimes the change to the top pipelining is forced by a layer (the conv layer) and as a consequence this causes all other layers to use resource strategy which is unintended. Decoupling the strategy from the pipeline style fixes this. The explicitly set resource strategy still forces the dataflow style to be used. The configuration can be explicitly set with PipelineStyle (values can be pipeline and dataflow), but normally the users would still continue using the Strategy config parameter. Fixes #699 and #759.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change which adds functionality)

Tests

I didn't add any extra tests. The best way to test this is to verify the generated HLS has the correct pragmas and parameters.h. I manually tested a whole lot of combinations of this. Checking the IR in python may also be possible, but I'm not sure if this will capture all combinations.

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 requested review from jmduarte and jmitrevs May 3, 2023 15:14
@vloncar vloncar added the please test Trigger testing by creating local PR branch label May 5, 2023
hls4ml/model/graph.py Outdated Show resolved Hide resolved
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels May 7, 2023
@jmitrevs
Copy link
Contributor

jmitrevs commented May 7, 2023

Do we understand why the tests failed? Otherwise, the requested change is quite minor.

@vloncar
Copy link
Contributor Author

vloncar commented May 7, 2023

Not sure why the tests failed, but certainly it is not due to the changes in the code. Since this started happening recently and we didn't change the CI configuration, it may be due to the increasing number of parallel tests each creating a new environment. Not sure why it doesn't deterministically fail. We need to investigate further. You can rerun the tests or run them locally to be sure that indeed nothing got broken. I ran locally, it all passed.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels May 8, 2023
@jmitrevs
Copy link
Contributor

jmitrevs commented May 8, 2023

Unless people object, I will merge this tomorrow. But is this for 0.8 or 0.7.1?

@vloncar
Copy link
Contributor Author

vloncar commented May 8, 2023

How about we merge #780, fix the capitalization issue observed in #782, release that as 0.7.1 and then start merging for 0.8.0 with this PR?

@jmitrevs jmitrevs added this to the v0.8.0 milestone May 12, 2023
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels May 13, 2023
@jmitrevs jmitrevs merged commit 4f0cea5 into fastmachinelearning:main May 14, 2023
calad0i pushed a commit to calad0i/hls4ml that referenced this pull request Jul 1, 2023
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.

Separate top level dataflow concept from layer strategy
2 participants