Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@swernli
Copy link
Contributor

@swernli swernli commented Nov 28, 2020

This comments out some code so that any dependency on an operation results in an edge to each specialization of that operation instead of just the specific specialization used. This is a patch to eliminate the issue, but we should consider either eliminating the commented out code and making this design change permanent (never prune dependencies of specializations even if they are unused), or allow for the pruning of both unused dependencies and the unused specialization itself.

Note that this also skips tests that fail due to the given work around.

Works around #757

This comments out some code so that any dependency on an operation results in an edge to each specialization of that operation instead of just the specific specialization used. This is a patch to eliminate the issue, but we should consider either eliminating the commented out code and making this design change permanent (never prune dependencies of specializations even if they are unused), or allow for the pruning of both unused dependencies and the unused specialization itself.

Note that this also skips tests that fail due to the given work around.
@swernli swernli requested a review from bettinaheim November 28, 2020 00:52
@swernli
Copy link
Contributor Author

swernli commented Nov 28, 2020

@bettinaheim, I've been able to confirm that this workaround fixes the issue I was seeing by using the generated packages and building code locally that uses AppyPauli and targets a provider.

@swernli
Copy link
Contributor Author

swernli commented Nov 28, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@swernli
Copy link
Contributor Author

swernli commented Nov 29, 2020

Annoyingly, this change means that the C# files produced will have more callables in them, which seems to be just enough to hit the timeout on the Linux perf tests. I tried a clean e2e build of main, and the Linux perf tests completed in 59 minutes, 4 seconds (see here). The limit is 60 minutes, so this PR is cancelled at 59 minutes and 59 seconds. From the logs, it looks like it gets cancelled at about 4/5ths of the way through, so making the test just slightly slower has resulted in hitting the timeout.

@bettinaheim
Copy link
Contributor

bettinaheim commented Nov 30, 2020

Given that this PR here microsoft/qsharp-runtime#450 also had the runtime perf cancelled, the timeout may not actually be related to this change.
Regarding a permanent fix for the pruning, I would prefer to properly capture the dependencies on individual specializations rather than being more pessimistic than we need to be, but also to always keep all functor specializations (and all dependencies) for a given set of type arguments in the call graph, independent on whether they are needed (a rewrite step may need them).

@bettinaheim
Copy link
Contributor

@swernli Closing this one, since I #764 deals with the same issue. I think we should have the feature branch there ready to merge to main soon, and otherwise we should cherry-pick that fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants