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

Updated partitioning function for PartitionFromDict #113

Merged
merged 3 commits into from
Dec 16, 2024

Conversation

bwintermann
Copy link
Contributor

I had recently tried building a larger model (~517 nodes) in FINN and got stuck at CreateDataflowPartition for a long time. I did some timing measurements and tracked the longest time spent down to PartitionFromDict. Since I supply a large partition dict for a large model, this step takes a long time. The changes I'd propose here keep the functionality but speed the usage of this transform up a bit:

If the node is not in the graph, the if block is never executed, but this check still runs over the whole graph for every key in the partitioning. Moving the check out before the loop enables an early return in case the node is not in the graph.

Since the index of the node in the graph doesn't change during the function execution, it does not need to be requested in every loop iteration, and so it can be moved out of the loop as well.

For smaller models and partitionings this is negligible, however it helped for my usecase and others might benefit from it as well.

@fpjentzsch
Copy link
Contributor

Nice catch, I didn't consider the performance aspect while writing this code ;-)

@maltanar
Copy link
Collaborator

Thanks @bwintermann ! Two minor things for the future:

  • For future PRs to qonnx please make sure you've run pre-commit: https://github.com/fastmachinelearning/qonnx?tab=readme-ov-file#linting
  • It's generally a good idea to start a branch for each PR, instead of opening it from the main branch of your own fork. This way you can put more custom features/commits on your own main branch while keeping the PR branch clean and self-contained.

@maltanar maltanar merged commit f08a869 into fastmachinelearning:main Dec 16, 2024
5 checks passed
@maltanar maltanar added this to the v0.4.0 milestone Dec 20, 2024
@bwintermann
Copy link
Contributor Author

@maltanar Sorry for missing to run pre-commit and opening a feature branch. I will keep it in mind for future PRs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants