Skip to content

Tutorial timestamps #2876

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

Closed
wants to merge 7 commits into from
Closed

Conversation

brcolli
Copy link

@brcolli brcolli commented May 18, 2024

Fixes #2848

Description

I wrote a script that goes to each .py and .rst file in beginner_source, intermediate_source, and advanced_source directories. The timestamp format is "Updated: HH:MM AM/PM, mm dd, yyyy". It appends the timestamp to the end of the file. At first, I added it to the line above or below the author line, but this had a lot of edge cases, depending on how a particular person wrote their tutorial. I could broach this again, but I figured that was an overengineered solution to a simple problem.

The timestamp is generated by looking at git logs for the specific file and seeing when it was last committed.
Alternative solution was looking at last modified time for the file, but that isn't concrete or modular.
Another possible alternative is adding a check in the pipeline to ensure the user manually inserts the line before they commit. This is the safest but least user friendly.

I created a .sh script to get called during the build-tutorials workflow. It is placed at the very beginning of the manager job or right before running make docs in the worker job.
This does add a few seconds to the build time, but nothing major.

I was able to test this on my forked repo with a simple runner, but I obviously couldn't test this in your dedicated pipeline.

Checklist

  • The issue that is being fixed is referred in the description (see above "Fixes #ISSUE_NUMBER")
  • Only one issue is addressed in this pull request
  • Labels from the issue that this PR is fixing are added to this pull request
  • No unnecessary issues are included into this pull request.

Copy link

pytorch-bot bot commented May 18, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/tutorials/2876

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit eb8f1d4 with merge base 2b0e464 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

Copy link
Contributor

@svekars svekars left a comment

Choose a reason for hiding this comment

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

Thank you for taking a stab at this! A few things:

  • I like that you are using git log. Can we please change the format from: "--format=%at" to '--date=format:%B %d, %Y' "--format=%at'"
  • I would really prefer the timestamp to appear at the top of the page, after the title perhaps, rather than at the bottom. Also, it should say: Last Updated: <timestamp>, with font: 300 italics.
  • I see that right now, it is only working for beginner/, intermediate/ and advanced/ paths. so any subdirs like beginner/basics are excluded. Can it be fixed?

# Get current timestamp
timestamp = get_last_commit_timestamp_for_file(file_path)
timestamp_line = f'{"# " if file_path.endswith("py") else ""}**Updated:** *{timestamp}*\n'
timestamp_pattern = r'\*\*Updated:\*\*\s\**\d{1,2}:\d{2} [AP]M, \w+ \d{1,2}, \d{4}\*'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather try to avoid regexes like this

Copy link
Author

Choose a reason for hiding this comment

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

I updated it! For my own knowledge, may I ask why? Is it because it's too specific? I was trying to be specific just so I didn't grab any lines unexpectedly from the tutorial files.


DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null && pwd )"

directories=("$SOURCEDIR/beginner_source" "$SOURCEDIR/intermediate_source" "$SOURCEDIR/advanced_source")
Copy link
Contributor

Choose a reason for hiding this comment

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

We have other directories as well and what if we add a new gallery directory, this won't work anymore? You could try perhaps to get them from the conf.py sphinx-gallery settings?

Copy link
Author

Choose a reason for hiding this comment

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

I updated it to get the files from the python script get_sphinx_filenames. This is what is used in conf.py, so can I assume that is a complete set?

@svekars svekars requested a review from malfet May 23, 2024 18:14
brcolli and others added 4 commits June 14, 2024 06:39
Co-authored-by: Svetlana Karslioglu <[email protected]>
Making get_sphinx_filenames print out files as well

Updating to set filenames to environ var

Resolving bash script to call files from correct directory

bash script to call files from proper directory
Changed date formatting to work with datetime
Removing input to update_timestamps_batch sh file
@brcolli
Copy link
Author

brcolli commented Jun 14, 2024

I also think it looks better if it's right after the title, but this comes with some caveats. Since we do not standardize the title across .py and .rst files, I would have to get very broad. Many people start files in many different ways. Often they follow the line with a separator like === or *** but some files don't have this. I would love your input, the best I came up with was "Define title as first line with alphabet characters, except in .rst files that begin with `". Is that an acceptable definition of "title"?

@brcolli
Copy link
Author

brcolli commented Jun 14, 2024

Finally, you requested that the font is weight 300. Is that the default weight used by our stylings for the HTML/CSS files?

@svekars
Copy link
Contributor

svekars commented Jul 1, 2024

I also think it looks better if it's right after the title, but this comes with some caveats. Since we do not standardize the title across .py and .rst files, I would have to get very broad. Many people start files in many different ways. Often they follow the line with a separator like === or *** but some files don't have this. I would love your input, the best I came up with was "Define title as first line with alphabet characters, except in .rst files that begin with `". Is that an acceptable definition of "title"?

You could implement a different approach - instead of trying to match that in .rst/.py, you could have timestamps say in a yml file and load them with .js based after the HTML <title> tag as we can't control which type of heading authors will implement.

Copy link

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the stale Stale PRs label Sep 27, 2024
@github-actions github-actions bot closed this Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

💡 [REQUEST] - Add a Timestamp to Every Tutorial
3 participants