Skip to content

Script: add profiling checkpoints to html()#2829

Open
ascholerChemeketa wants to merge 1 commit intoPreTeXtBook:masterfrom
ascholerChemeketa:py-profiling-hooks
Open

Script: add profiling checkpoints to html()#2829
ascholerChemeketa wants to merge 1 commit intoPreTeXtBook:masterfrom
ascholerChemeketa:py-profiling-hooks

Conversation

@ascholerChemeketa
Copy link
Copy Markdown
Contributor

Pulled out from: #2329


This pull request adds a lightweight timing and profiling utility to the pretext/lib/pretext.py module, enabling optional logging of elapsed time at key stages of the HTML build process. The main change is the introduction of a Stopwatch class and its integration into the html() function, allowing developers to profile and monitor build performance when needed.

Profiling and Logging Enhancements:

  • Introduced a Stopwatch class for measuring and logging elapsed time between key events, with optional logging controlled by the profile.py string parameter. (pretext/lib/pretext.py)
  • Integrated the Stopwatch into the html() function, adding time logs at major build steps such as loading publisher variables, placing Runestone services, copying managed directories, copying CSS/JS, running XSLT, and completing the build. (pretext/lib/pretext.py) [1] [2] [3] [4] [5]

Dependency Update:

  • Added an import for the standard time module to support the new timing functionality. (pretext/lib/pretext.py)

@rbeezer
Copy link
Copy Markdown
Collaborator

rbeezer commented Apr 12, 2026

Looking good visually. You didn't like the comment that equaled the function name? ;-)

@rbeezer
Copy link
Copy Markdown
Collaborator

rbeezer commented Apr 12, 2026

And shoulda said - thanks for breaking this out.

@rbeezer
Copy link
Copy Markdown
Collaborator

rbeezer commented Apr 12, 2026

I was confused by "profile.py" when I did a visual review. Do we have a module named "profile.py"? Or is it from some associated library? And why does it matter? I'll need to chase that down, I thought. How about "html.profile"? Yes, it is all in the Python, but it is also the HTML build being profiled. More consistent with names in use otherwise.

Claude flagged this as well. That review coming next. Mostly nitpicking stuff, but maybe something you would like to react to.

We have a developer's section in the Guide. Let's record the existence of this switch there somewhere with just a single sentence. Likely no great/obvious place, so don't sweat it. It'll get reorganized eventually.

You can pile on new commits, if you keep documentation on the last one by itself, and I'll clean up.

@rbeezer
Copy link
Copy Markdown
Collaborator

rbeezer commented Apr 12, 2026

Thanks for breaking this out from #2329, much easier to evaluate independently.

Clean and useful. A few observations:

Stringparam naming: The param is profile.py here but was profile-py in #2329. The dot-py reads like a filename rather than a parameter name. Is profile.py intentional, or should it be profile-py (or something else entirely)? Just want to make sure the two PRs agree.

Minor:

  • The diff removes a blank line and the # build or copy theme comment between copy_html_js and build_or_copy_theme. Not a problem, but unrelated to the profiling work — could be a separate cleanup or just left alone.
  • reset() is only called internally — could be _reset(), but the class is small enough that it doesn't matter much.

Otherwise looks good to merge once the naming question is settled.

Claude Opus 4.6 acting as a review assistant for Rob Beezer

@ascholerChemeketa
Copy link
Copy Markdown
Contributor Author

How about debug.profile or debug.profile-py ?

I could see adding timestamp logging to other pathways and wanting to use the same flag to say "show me timing" while debugging.

@rbeezer
Copy link
Copy Markdown
Collaborator

rbeezer commented Apr 13, 2026

I like debug.profile. Thanks.

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.

2 participants