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.

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

Intro Level Tutorial #590

Merged
merged 5 commits into from
May 20, 2024
Merged

Intro Level Tutorial #590

merged 5 commits into from
May 20, 2024

Conversation

jcarnaxide
Copy link
Contributor

Issue number of the reported bug or feature request: #

Describe your changes
We have worked on an introductory level course for people to get familiar with Memray.

Testing performed
We ran make docs-live to view the new RST files, and to see everything is rendered properly.
We also ran the exercises via instructions specified and was able to get through the whole tutorial.

Additional context
It is worth noting, that I plan to update the instructions to include running the tutorial via codespaces, but we stuck with simple venv/pip for this first PR.

Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

I haven't proofread any of the actual tutorials yet, but I've got a few comments on the organization and structure, to start with:

docs/intro.rst Outdated
@@ -0,0 +1,23 @@
Introduction
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename merge this file and "contents.rst" into one file. We've already got the "overview.html" page as an intro to Memray, so what we need here is just an overview of the structure and contents of the tutorials, what prerequisites they have, how they're structured, etc. Less "intro to Memray", more "intro to these tutorials".

Something like:

The tutorials in this section of the docs provide a gentle introduction to debugging memory issues using the Memray profiler. You'll learn how to use basic features like (...) as well as some more advanced features like (...). There are 3 different tutorials: ... You should follow them in the order they're presented here, as each builds on concepts introduced by the last.

Etc etc. We just want to explain how to use the tutorials, and what they are, and what they do (and maybe don't) cover, and what you'll learn by using them.

Also, the merged file should be called "tutorials.rst" or "tutorials/index.rst" or something like that (the filename becomes part of the URL, so it'll matter for links).

I think I most like the idea of pushing all of these .rst files down into the docs/tutorials, and having them as docs/tutorials/index.rst (which should become accessible as either https://bloomberg.github.io/memray/tutorials or as https://bloomberg.github.io/memray/tutorials/index.html), and then having the other tutorial pages be in that same folder. At which point, https://bloomberg.github.io/memray/tutorials/exercise_1.html won't be a terrible name, though I wonder if https://bloomberg.github.io/memray/tutorials/1.html or https://bloomberg.github.io/memray/tutorials/fibonacci.html might be better.

docs/index.rst Outdated
Comment on lines 36 to 46

.. toctree::
:hidden:
:caption: Hands-on Tutorial

intro
contents
exercise_1
exercise_2
exercise_3
additional_features
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's actually yank this up higher - I think I'd like to create a new section called "Concepts" and drop run through performance into it, and then lift this section up to between getting_started and run. That way the sidebar will be ordered as:

  • Overview
  • Getting started
  • Hands-on Tutorial
    • About these tutorials
    • Exercise 1 - ...
    • Exercise 2 - ...
    • Exercise 3 - ...
    • What to learn about next
  • Concepts
    • run
    • python_allocators
      ...
I think that's a pretty nice organization, and should do a good job of highlighting these new docs. We should also make getting_started.html suggest that the next steps after you've generated your first flame graph would be to either try the tutorials or to learn more about `memray run`, with links to either the tutorial or skipping over it to the concepts section.

@jcarnaxide
Copy link
Contributor Author

@godlygeek I have addressed your comments for restructuring, as well as combining the introduction to the tutorials.
@statkute please have a look at the changes I have made.

I think we should still update the rst page for additional features inside the tutorials directory.

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.70%. Comparing base (41248ed) to head (8bd476d).
Report is 42 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #590      +/-   ##
==========================================
+ Coverage   92.55%   92.70%   +0.14%     
==========================================
  Files          91       92       +1     
  Lines       11304    11234      -70     
  Branches     1581     2055     +474     
==========================================
- Hits        10462    10414      -48     
+ Misses        837      820      -17     
+ Partials        5        0       -5     
Flag Coverage Δ
cpp 92.70% <ø> (+6.76%) ⬆️
python_and_cython 92.70% <ø> (-3.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jcarnaxide jcarnaxide force-pushed the tutorial branch 3 times, most recently from 70e3bca to 24dace0 Compare May 10, 2024 20:18
@statkute
Copy link
Contributor

@godlygeek I have addressed your comments for restructuring, as well as combining the introduction to the tutorials. @statkute please have a look at the changes I have made.

I think we should still update the rst page for additional features inside the tutorials directory.

I've now updated a couple internal links that stopped working due to the new tutorials/ directory. Otherwise looks great-- thanks for the fixes while I was out @jcarnaxide !

I've also added a brief section that encourages to explore the Concepts section after finishing the hands-on tutorial (under additional features).

@godlygeek we should be good for another look whenever you have the time -- thank you!

@jcarnaxide jcarnaxide requested a review from godlygeek May 15, 2024 13:20
statkute and others added 5 commits May 20, 2024 00:07
Signed-off-by: Gintare Statkute <statkute.g@gmail.com>
Signed-off-by: Jesse Carnaxide <jcarnaxide@gmail.com>
Pull tutorial `.rst` files down into separate folder.
Also, reorganize the side bar.

Signed-off-by: Jesse Carnaxide <jcarnaxide@gmail.com>
Signed-off-by: Gintare Statkute <gstatkute@bloomberg.net>
Update the tutorials to follow our docs' typical typographical
conventions, improve some wording here and there, use American spellings
rather than British spellings for consistency with the rest of our
documentation, and avoid using any more inline HTML than necessary for
handling expandable spans for solutions.

Signed-off-by: Matt Wozniski <godlygeek@gmail.com>
Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

I've made some edits, after which this LGTM.

I'm going to land this, so that it can be used by our Sprinters at PyCon US tomorrow.

@jcarnaxide and @statkute please take a look at the last commit on this PR if you get a chance, and open an issue or a new PR if you spot any mistakes I've introduced. Sorry for not giving you time to review before I landed it!

@godlygeek godlygeek enabled auto-merge (rebase) May 20, 2024 04:13
@godlygeek godlygeek merged commit 21a0a06 into bloomberg:main May 20, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants