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

unify doc preview URLs #3917

Merged
merged 2 commits into from
May 8, 2023
Merged

unify doc preview URLs #3917

merged 2 commits into from
May 8, 2023

Conversation

pmeier
Copy link
Contributor

@pmeier pmeier commented Mar 23, 2023

Closes #3894. This needs to be merged in unison with a PR on PyTorch core and torchvision. I'll link them here when they are up.

@vercel
Copy link

vercel bot commented Mar 23, 2023

@pmeier is attempting to deploy a commit to the Meta Open Source Team on Vercel.

A member of the Team first needs to authorize it.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 23, 2023
Comment on lines +13 to +15
export const DOCS_URL = "https://docs-preview.pytorch.org";
export const PYTHON_DOCS_PATH = "index.html";
export const CPP_DOCS_PATH = "cppdocs/index.html";
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've removed the leading and trailing / here, because I found it rather hard to read below when I needed a /.

@pmeier
Copy link
Contributor Author

pmeier commented Mar 23, 2023

@pmeier pmeier requested a review from seemethere March 23, 2023 09:25
@pmeier pmeier marked this pull request as ready for review March 23, 2023 09:25
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Please plan carefully how you stage those changes, in order not to break folks with their work in progress.

@pmeier
Copy link
Contributor Author

pmeier commented Mar 24, 2023

In the light of pytorch/pytorch#97433 (review), I've changed the PyTorch PR to not change the path just yet, but additionally upload to the new path we want to go for. As @malfet explains there, this means we can have a grace period where folks will already upload to the new location and if we switch the bot to display the new link this also works on these PRs even though they haven't been rebased.

@malfet proposed a grace period of a week. I'm ok with that. Any concerns?

@pmeier
Copy link
Contributor Author

pmeier commented Mar 24, 2023

With the new plan, TorchVision also needs a patch: pytorch/vision#7459

@vercel
Copy link

vercel bot commented Mar 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
torchci ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 27, 2023 at 7:00PM (UTC)

pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Mar 29, 2023
Sister patch for pytorch/test-infra#3917. TL;DR this moves the doc preview scheme to $OWNER/$REPO rather than $REPO that we are currently rolling.
Pull Request resolved: #97433
Approved by: https://github.com/seemethere, https://github.com/malfet
@pmeier
Copy link
Contributor Author

pmeier commented Mar 29, 2023

Both the PyTorch and TorchVision PR have been merged. Meaning, we can move ahead with this PR in a week or so to have a proper grace period.

@pmeier
Copy link
Contributor Author

pmeier commented Apr 13, 2023

Grace period is now a little over 2 weeks so we are good to go here.

I would start the rollout by merging pytorch/pytorch#99032 since that takes the longest time. Afterwards, these three things need to happen:

  1. Merge remove double doc upload after CloudFront fix vision#7516
  2. Merge this PR
  3. Fix the CloudFront configuration to not swallow the leading pytorch/ in the path

@atalman atalman merged commit 611cd8e into pytorch:main May 8, 2023
pmeier added a commit that referenced this pull request May 10, 2023
#3917 was merged so we no longer need to upload to two paths. Note that
the new scheme is `{OWNER}/{REPO}/{PR}` and this was dubbed "legacy".
Thus, we are actually removing the other one and dropping the legacy
label.
@pmeier pmeier deleted the unify-doc-preview branch May 31, 2023 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify HUD and docs preview paths?
6 participants