Skip to content

[pytorch] remove boilerplate setQEngine() from PyTorch mobile predictors#34556

Closed
ljk53 wants to merge 1 commit intogh/ljk53/121/basefrom
gh/ljk53/121/head
Closed

[pytorch] remove boilerplate setQEngine() from PyTorch mobile predictors#34556
ljk53 wants to merge 1 commit intogh/ljk53/121/basefrom
gh/ljk53/121/head

Conversation

@ljk53
Copy link
Copy Markdown
Contributor

@ljk53 ljk53 commented Mar 10, 2020

Stack from ghstack:

Summary:
According to
#34012 (comment),
this at::globalContext().setQEngine(at::QEngine::QNNPACK); call isn't
really necessary for mobile.

In Context.cpp it selects the last available QEngine if the engine isn't
set explicitly. For OSS mobile prebuild it should only include QNNPACK
engine so the default behavior should already be desired behavior.

It makes difference only when USE_FBGEMM is set - but it should be off
for both OSS mobile build and internal mobile build.

Differential Revision: D20374522

Summary:
According to
#34012 (comment),
this `at::globalContext().setQEngine(at::QEngine::QNNPACK);` call isn't
really necessary for mobile.

In Context.cpp it selects the last available QEngine if the engine isn't
set explicitly. For OSS mobile prebuild it should only include QNNPACK
engine so the default behavior should already be desired behavior.

It makes difference only when USE_FBGEMM is set - but it should be off
for both OSS mobile build and internal mobile build.

[ghstack-poisoned]
ljk53 added a commit that referenced this pull request Mar 10, 2020
Summary:
According to
#34012 (comment),
this `at::globalContext().setQEngine(at::QEngine::QNNPACK);` call isn't
really necessary for mobile.

In Context.cpp it selects the last available QEngine if the engine isn't
set explicitly. For OSS mobile prebuild it should only include QNNPACK
engine so the default behavior should already be desired behavior.

It makes difference only when USE_FBGEMM is set - but it should be off
for both OSS mobile build and internal mobile build.

ghstack-source-id: 139184c
Pull Request resolved: #34556
@ljk53 ljk53 requested review from ezyang, supriyar and xta0 March 10, 2020 22:19
@xta0
Copy link
Copy Markdown
Contributor

xta0 commented Mar 10, 2020

"In Context.cpp it selects the last available QEngine if the engine isn't
set explicitly. For OSS mobile prebuild it should only include QNNPACK
engine so the default behavior should already be desired behavior."

We also include NNPACK for OSS build, don't know if that counted as an engine.

Copy link
Copy Markdown
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

I don't know if it's right, but I love deleted code!

@ljk53
Copy link
Copy Markdown
Contributor Author

ljk53 commented Mar 10, 2020

"In Context.cpp it selects the last available QEngine if the engine isn't
set explicitly. For OSS mobile prebuild it should only include QNNPACK
engine so the default behavior should already be desired behavior."

We also include NNPACK for OSS build, don't know if that counted as an engine.

it's not counted as "q"-engine :)

@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented Mar 11, 2020

💊 CircleCI build failures summary and remediations

As of commit 8195861 (more details on the Dr. CI page):


None of the build failures appear to be your fault 💚


  • 2/2 broken upstream at merge base 259d729 since Mar 10

    Please rebase on the viable/strict branch (expand for instructions)

    Since your merge base is older than viable/strict, run these commands:

    git fetch origin viable/strict
    git rebase viable/strict
    

    Check out the recency history of this "viable master" tracking branch.


🚧 2 upstream failures:

These were probably caused by upstream breakages:


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

This comment has been revised 16 times.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@ljk53 merged this pull request in 7aca9af.

@facebook-github-bot facebook-github-bot deleted the gh/ljk53/121/head branch March 14, 2020 14:16
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
…ors (pytorch#34556)

Summary:
Pull Request resolved: pytorch#34556

According to
pytorch#34012 (comment),
this `at::globalContext().setQEngine(at::QEngine::QNNPACK);` call isn't
really necessary for mobile.

In Context.cpp it selects the last available QEngine if the engine isn't
set explicitly. For OSS mobile prebuild it should only include QNNPACK
engine so the default behavior should already be desired behavior.

It makes difference only when USE_FBGEMM is set - but it should be off
for both OSS mobile build and internal mobile build.

Test Plan: Imported from OSS

Differential Revision: D20374522

Pulled By: ljk53

fbshipit-source-id: d4e437a03c6d4f939edccb5c84f02609633a0698
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants