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

Update the vf2 call limit to scale with the size of the input graph #8246

Closed
wants to merge 8 commits into from

Conversation

HM0880
Copy link

@HM0880 HM0880 commented Jun 27, 2022

Summary

Update vf2 algorithm call limit per #7705.

Details and comments

  • The code for the layout passes has been updated recently, and I am not entirely sure what the difference between call_limit and vf2_call_limit is intended to be.

  • I also do not know if self.call_limit is the preferred way to pass the call_limit parameter.

@HM0880 HM0880 requested a review from a team as a code owner June 27, 2022 21:07
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@HuangJunye HuangJunye added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 28, 2022
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for starting this, I have a couple questions. I see the scaling factor you inserted into the VF2Layout pass, but it looks like the intent is to make the internal call limit solely a function of the subgraph size if not explicitly passed in. Were you planning to add any scaling based on the optimization level? Also I'm unclear on where the call_limit parameter is originating from now in this PR. It looks like you're removing the hard coding and passing it in via the pass manager config, but that doesn't actually seem to be added to the pass manager config class. So I'm curious at a high level what is the expected workflow here? What is the intended use of call_limit at the pass manager level used for with your change (if it's still used at all)? I think maybe it would be helpful to have in the commit message an outline of how this change and the expected workflow with it (this would also be good as part of a release note documenting the change).

To answer your question about call_limit and vf2_call_limit. They're the same thing, vf2_call_limit is just an outer variable which also gets passed to the routing stage generator function and it defaults to None so that we avoid running VF2PostLayout if a manual layout method was specified. This was done to deduplicate the call_limit handling between the pre and post vf2 layout functions (originally both just had the same hard coded value).

@@ -81,6 +81,7 @@ def level_1_pass_manager(pass_manager_config: PassManagerConfig) -> StagedPassMa
unitary_synthesis_plugin_config = pass_manager_config.unitary_synthesis_plugin_config
timing_constraints = pass_manager_config.timing_constraints or TimingConstraints()
target = pass_manager_config.target
call_limit = pass_manager_config.call_limit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you adding this to the passmanager config somewhere? I don't see that change anywhere

HM0880 added 3 commits June 29, 2022 16:20
Set `call_limit` to be the maximum of either the retworkx 0.10.2 call
limits or a scaled factor of the exponential call limit calculated in
`./qiskit/transpiler/passes/layout/vf2_layout.py`.  The scaling factor
grows with the optimization level: 0.25 for level 1, 0.50 for level 2,
and 0.75 for level 3.
@HM0880
Copy link
Author

HM0880 commented Jun 29, 2022

@mtreinish

I see the scaling factor you inserted into the VF2Layout pass, but it looks like the intent is to make the internal call limit solely a function of the subgraph size if not explicitly passed in. Were you planning to add any scaling based on the optimization level?

Added scaling in 7950406.

Also I'm unclear on where the call_limit parameter is originating from now in this PR. It looks like you're removing the hard coding and passing it in via the pass manager config, but that doesn't actually seem to be added to the pass manager config class. So I'm curious at a high level what is the expected workflow here? What is the intended use of call_limit at the pass manager level used for with your change (if it's still used at all)?

Honestly, I wasn't sure how to get call_limit from vf2_layout.py into the level[1,2,3].py functions. I shoved call_limit into the pass manager config since the code style didn't use self.whatever, but that choice was just a placeholder.

I think maybe it would be helpful to have in the commit message an outline of how this change and the expected workflow with it (this would also be good as part of a release note documenting the change).

I imagine that most users will not set call_limit and instead rely on the default values; the default values will be used when call_limit=None. If the user does set the call limit, then the user-supplied value is used. Does this answer your workflow question, or were you thinking of something else?


To answer your question about call_limit and vf2_call_limit. They're the same thing, vf2_call_limit is just an outer variable which also gets passed to the routing stage generator function and it defaults to None so that we avoid running VF2PostLayout if a manual layout method was specified. This was done to deduplicate the call_limit handling between the pre and post vf2 layout functions (originally both just had the same hard coded value).

OK. I set call_limit and vf2_call_limit to the same call_limit value, which seems fine. Let me know if it is not.

@javabster
Copy link
Contributor

Closing this PR as it seems more research is needed before proceeding, we can always open it again at a later date when more information is available

@javabster javabster closed this Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants