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

Add SnakeViz profiling example #1657

Merged
merged 4 commits into from
Jun 21, 2021
Merged

Add SnakeViz profiling example #1657

merged 4 commits into from
Jun 21, 2021

Conversation

KevinCawley
Copy link
Contributor

@KevinCawley KevinCawley commented Jun 17, 2021 •
edited
Loading

This is a notebook that shows a sample output from SnakeViz.

Description

I made a notebook that runs SnakeViz on run_tardis('tardis_example.yml'). While the HTML cannot be accurately displayed inline, a PNG of the output is displayed below. There is text for explanation for each cell.

Motivation and context

Shows an example of how to profile TARDIS.

How has this been tested?

  • Testing pipeline.
  • Other.

Examples

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@tardis-bot
Copy link
Contributor

tardis-bot commented Jun 17, 2021 •
edited by andrewfullard
Loading

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

@KevinCawley
Copy link
Contributor Author

How it will look upon merge. If Dhruv's PR #1633 is merged, then the long output from Cell 2 will dissapear.

https://kevincawley.github.io/tardis/branch/snakeviz_notebook/io/output/profiling_example.html

andrewfullard
andrewfullard approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@andrewfullard andrewfullard 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, can the snakeviz image include the table?

@KevinCawley
Copy link
Contributor Author

KevinCawley commented Jun 17, 2021 •
edited
Loading

By the table do you mean the cProfile information? I was unable to get it to due to constraints on my virtual box with taxing pictures. I chose to prioritize the visualization of the data as I had to choose one.

@codecov
Copy link

codecov bot commented Jun 17, 2021 •
edited
Loading

Codecov Report

Merging #1657 (d841a31) into master (b69b093) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1657   +/-   ##
=======================================
  Coverage   61.93%   61.93%           
=======================================
  Files          62       62           
  Lines        5729     5729           
=======================================
  Hits         3548     3548           
  Misses       2181     2181

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b69b093...d841a31. Read the comment docs.

isaacgsmith
isaacgsmith requested changes Jun 17, 2021
View reviewed changes
Copy link
Member

@isaacgsmith isaacgsmith left a comment

Choose a reason for hiding this comment

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

Can you commit the notebook with cells not executed?

@andrewfullard andrewfullard self-requested a review June 17, 2021 19:10
@tardis-bot
Copy link
Contributor

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

@KevinCawley
Copy link
Contributor Author

I could try, but that would be another commit. Are you fine w/ that?

@isaacgsmith
Copy link
Member

I could try, but that would be another commit. Are you fine w/ that?

Yeah. It is important that notebooks are committed without any output.

@KevinCawley
Copy link
Contributor Author

I feel like that should be put somewhere.

@KevinCawley KevinCawley force-pushed the snakeviz_notebook branch 2 times, most recently from ae94374 to eab9a17 Compare June 17, 2021 19:27
@KevinCawley
Copy link
Contributor Author

Do not merge until I have gotten rid of some of these commits, thanks.

@isaacgsmith
Copy link
Member

isaacgsmith commented Jun 17, 2021 •
edited
Loading

@KevinCawley your latest doc preview action failed

https://github.com/KevinCawley/tardis/actions/runs/947585168

@isaacgsmith
Copy link
Member

I feel like that should be put somewhere.

It should be. I am going to put it somewhere in one of my upcoming PRs.

@KevinCawley
Copy link
Contributor Author

KevinCawley commented Jun 18, 2021 •
edited
Loading

I am able to build locally. I think the error comes from SnakeViz not being downloaded by the TARDIS env, so the user must first install it. I will add that to the notebook, but will this affect the docs?

@KevinCawley
Copy link
Contributor Author

The error stems from SnakeViz not being inside of the TARDIS environment.

@andrewfullard
Copy link
Contributor

It appears that <script>document.getElementById("snakeviz-4fcfc466-d039-11eb-88f7-000d3a7cea3b").setAttribute("src", "http://" + document.location.hostname + ":8080/snakeviz/%2Ftmp%2Ftmpiy9oiaxj")</script> is where the display is failing, probably because of how snakeviz accesses the data. This may be solvable with pipeline changes. For now, it just leaves a blank space in the notebook.

@KevinCawley
Copy link
Contributor Author

Yes. There is nothing for it to display, but the white space still shows up.

@KevinCawley
Copy link
Contributor Author

KevinCawley commented Jun 21, 2021 •
edited
Loading

@smithis7
https://kevincawley.github.io/tardis/branch/snakeviz_notebook/io/output/profiling_example.html

@andrewfullard andrewfullard merged commit ff7c344 into tardis-sn:master Jun 21, 2021
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
@KevinCawley
Add SnakeViz profiling example (tardis-sn#1657)
159998d 
* Snakeviz notebook

* Modify env file

* [build docs]

* [Build docs]

Note that the current whitespace is unfortunate but meeehhh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isaacgsmith isaacgsmith isaacgsmith requested changes

@andrewfullard andrewfullard andrewfullard approved these changes

@wkerzendorf wkerzendorf Awaiting requested review from wkerzendorf

Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KevinCawley @tardis-bot @isaacgsmith @andrewfullard