Skip to content

Conversation

swolchok
Copy link
Contributor

This script checks that all files in our c10 copy are in sync with our pinned version of PyTorch.

Test Plan: 1) detected us being out of sync when I wrote this PR
2) CI to run it and make sure that we are clean now

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@swolchok
Copy link
Contributor Author

swolchok commented Apr 23, 2025

Stack from ghstack (oldest at bottom):

@swolchok swolchok requested a review from tarun292 as a code owner April 23, 2025 21:35
Copy link

pytorch-bot bot commented Apr 23, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10413

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

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

@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 Apr 23, 2025
swolchok added a commit that referenced this pull request Apr 23, 2025
This script checks that all files in our c10 copy are in sync with our pinned version of PyTorch.

Test Plan: 1) detected us being out of sync when I wrote this PR
2) CI to run it and make sure that we are clean now


ghstack-source-id: 63e3e79
ghstack-comment-id: 2825552562
Pull-Request-resolved: #10413
@swolchok swolchok temporarily deployed to upload-benchmark-results April 23, 2025 22:28 — with GitHub Actions Inactive
Copy link
Contributor

@kimishpatel kimishpatel left a comment

Choose a reason for hiding this comment

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

This is great

# LICENSE file in the root directory of this source tree.

set -exu
ls pytorch/.git || git clone https://github.com/pytorch/pytorch.git
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: --depth 1? or will the commit checkout not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty sure --depth 1 will break checking out specific commit. also checked and there's no way to clone just a specific commit

Comment on lines 27 to 31
set -ex
git clone https://github.com/pytorch/pytorch.git
pushd pytorch || return
git checkout "$(< ../.ci/docker/ci_commit_pins/pytorch.txt)"
popd
Copy link
Contributor

Choose a reason for hiding this comment

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

Given check_c10_sync.sh already handles this, why do we need to do this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops

pushd pytorch
git checkout "$(< ../.ci/docker/ci_commit_pins/pytorch.txt)"
popd
"$(dirname "${BASH_SOURCE[0]}")"/diff_c10_mirror_with_pytorch.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"$(dirname "${BASH_SOURCE[0]}")"/diff_c10_mirror_with_pytorch.sh
"$(git rev-parse --show-toplevel)/.ci/scripts/diff_c10_mirror_with_pytorch.sh"

mega nit: this implicitly assumes theses scripts will always be adjacent

# This source code is licensed under the BSD-style license found in the
# LICENSE file in the root directory of this source tree.

$(dirname "${BASH_SOURCE[0]}")/compare_dirs.sh runtime/core/portable_type/c10/c10 pytorch/c10
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this script for just a single line? Can we just use the line directly in check_c10_sync.sh?

If not, I guess we want to either exec this or set -eu up top

Base automatically changed from gh/swolchok/423/head to main April 25, 2025 22:01
[ghstack-poisoned]
swolchok added a commit that referenced this pull request Apr 25, 2025
This script checks that all files in our c10 copy are in sync with our pinned version of PyTorch.

Test Plan: 1) detected us being out of sync when I wrote this PR
2) CI to run it and make sure that we are clean now

ghstack-source-id: 64b1b89
ghstack-comment-id: 2825552562
Pull-Request-resolved: #10413
Copy link
Contributor

@jathu jathu left a comment

Choose a reason for hiding this comment

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

Please address CI paths comments

paths:
- .ci/docker/ci_commit_pins/pytorch.txt
- .ci/scripts/compare_dirs.sh
- .ci/scripts/diff_c10_mirror_with_pytorch.sh
Copy link
Contributor

@jathu jathu Apr 25, 2025

Choose a reason for hiding this comment

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

Delete — diff_c10_mirror_with_pytorch no longer exists


on:
pull_request:
paths:
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also add runtime/core/portable_type/c10 to this list

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Apr 25, 2025
This script checks that all files in our c10 copy are in sync with our pinned version of PyTorch.

Test Plan: 1) detected us being out of sync when I wrote this PR
2) CI to run it and make sure that we are clean now

ghstack-source-id: 8eb6700
ghstack-comment-id: 2825552562
Pull-Request-resolved: #10413
@swolchok swolchok merged commit 6b877de into main Apr 25, 2025
171 checks passed
@swolchok swolchok deleted the gh/swolchok/425/head branch April 25, 2025 23:52
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. topic: not user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants