Skip to content

Conversation

Krovatkin
Copy link
Contributor

No description provided.

@facebook-github-bot facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Feb 9, 2021
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Feb 9, 2021

💊 CI failures summary and remediations

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


💚 💚 Looks good so far! There are no failures yet. 💚 💚


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

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #52001 (ff49c12) into master (0410cba) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #52001      +/-   ##
==========================================
- Coverage   80.77%   80.77%   -0.01%     
==========================================
  Files        1953     1953              
  Lines      214064   214072       +8     
==========================================
+ Hits       172913   172918       +5     
- Misses      41151    41154       +3     

@Krovatkin Krovatkin requested a review from eellison February 10, 2021 18:01
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@Krovatkin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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 good as a debug mechanism, I think you should add more warnings or change the name of the function to indicate this is not for use outside of testing

GraphExecutorState getDebugState() override;
~ProfilingGraphExecutorImpl() override = default;

void flushCompilationCache() {
Copy link
Contributor

Choose a reason for hiding this comment

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

you might want to add some underscores here, or call it debugFlushCompilationCache. We dont want anyone else to use this really..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll rename it to debug


void flushCompilationCache() {
pr_.reset();
fallback_plan_.reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some of these have mutexes that you're not accessing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes! Thank you for pointing this out!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@Krovatkin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@Krovatkin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@Krovatkin merged this pull request in 0019a20.

xsacha pushed a commit to xsacha/pytorch that referenced this pull request Mar 31, 2021
Summary: Pull Request resolved: pytorch#52001

Reviewed By: eellison

Differential Revision: D26371876

Pulled By: Krovatkin

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

Labels

cla signed Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants