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.

Sign up for GitHub

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

Jump to bottom

adding profile_ivalue #47666

Closed
wants to merge 5 commits into from
Closed

adding profile_ivalue #47666

wants to merge 5 commits into from

Conversation

jjsjann123
Copy link
Collaborator

@jjsjann123 jjsjann123 commented Nov 10, 2020 •
edited

Stack from ghstack:

Differential Revision: D25255573

@jjsjann123
adding profile_ivalue
a4f6239 
[ghstack-poisoned]
This was referenced Nov 10, 2020
Closed
Closed
Closed
@facebook-github-bot facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Nov 10, 2020
@jjsjann123 jjsjann123 mentioned this pull request Nov 10, 2020
Closed
@dr-ci
Copy link

dr-ci bot commented Nov 10, 2020 •
edited

💊 CI failures summary and remediations

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

✅ None of the CI failures appear to be your fault 💚

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

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

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

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

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

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 or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 20 times.

eellison
eellison reviewed Nov 17, 2020
View reviewed changes
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

Looks fine to me, wait on @Krovatkin

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp Outdated
Operator(
prim::profile_ivalue,
[](const Node* node) -> Operation {
auto callback = node->cast<ProfileIValueOp>()->getCallback();
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason to call getCallBack here ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. It probably should be removed now since profile is lowered to interpreter's instruction. Same with prim::profile & prim::profile_optional 😛

jjsjann123 added a commit to jjsjann123/pytorch that referenced this pull request Nov 20, 2020
@jjsjann123
adding profile_ivalue
0aa2eed 
ghstack-source-id: 8193e6ae6ed1e59d886e5ba74db2ebe7e1f8936d
Pull Request resolved: pytorch#47666
eellison
eellison approved these changes Dec 2, 2020
View reviewed changes
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

LGTM, some tests never hurt but i guess this is used in the next PR

@jjsjann123
Copy link
Collaborator Author

@Krovatkin
I know we are still going through the refactor PR. Meanwhile, this one should be good to go. Can we try to import it?

@jjsjann123
Copy link
Collaborator Author

Having said that, if there's any real failure marked from the merge conflict in CI, feel free to point it to me so I can try to fix.

@facebook-github-bot
Copy link
Contributor

@Krovatkin merged this pull request in a6fa3b2.

@facebook-github-bot facebook-github-bot deleted the gh/jjsjann123/2/head branch December 13, 2020 15:17
jjsjann123 added a commit to jjsjann123/nvfuser that referenced this pull request Oct 29, 2022
@jjsjann123 @facebook-github-bot
adding profile_ivalue (#47666)
53a7594 
Summary: Pull Request resolved: pytorch/pytorch#47666

Test Plan: Imported from OSS

Reviewed By: ZolotukhinM

Differential Revision: D25255573

Pulled By: Krovatkin

fbshipit-source-id: 5d8753e4040a3d96105d28d26728125947c7a638
jjsjann123 added a commit to jjsjann123/nvfuser that referenced this pull request Nov 10, 2022
@jjsjann123 @facebook-github-bot
adding profile_ivalue (#47666)
14a2a83 
Summary: Pull Request resolved: pytorch/pytorch#47666

Test Plan: Imported from OSS

Reviewed By: ZolotukhinM

Differential Revision: D25255573

Pulled By: Krovatkin

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

@eellison eellison eellison approved these changes

@apaszke apaszke Awaiting requested review from apaszke

Assignees
No one assigned
Labels
cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue open source
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jjsjann123 @facebook-github-bot @eellison @pytorchbot