Skip to content

Create _export directory in torchtune #2011

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

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

Jack-Khuu
Copy link
Contributor

@Jack-Khuu Jack-Khuu commented Nov 15, 2024

Summary: Creates a placeholder directory for adding exportable torchtune components

PyTorch Edge and ExecuTorch have been working with torchtune folk to create exportable variants of torchtune modules these past few months. The variants will be used to demonstrate new inference modes via torchtune components in the near future

This new directory will serve to host these components and will be leveraged in other repos, such as ExecuTorch and torchchat. These components are NOT meant for use during fine-tuning

Note that this PR only creates the _export directory to allow parallel contributions with minimal merge conflict.

Reviewed By: larryliu0820

Differential Revision: D65982921

Copy link

pytorch-bot bot commented Nov 15, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/2011

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 af1bfc1 with merge base bd89bb8 (image):
💚 Looks good so far! There are no failures yet. 💚

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 Nov 15, 2024
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D65982921

@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D65982921

Summary:

Creates a placeholder directory for adding exportable torchtune components

Reviewed By: larryliu0820, dvorjackz

Differential Revision: D65982921
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D65982921

@joecummings
Copy link
Contributor

If this has been already discussed with other members of the team I apologize, but what's the context here? Why do exportable components need to live in torchtune?

@RdoubleA
Copy link
Collaborator

Hey @Jack-Khuu and @dvorjackz, just caught up on the context of this change. Would be awesome if you could detail in the PR summary the motivation for this folder (clean dependency graph for torchchat and executorch for exportable modules) and what the plan is for CI. From what I gathered, the CI will live in torchtune and code quality/updating the CI for _export is largely the responsibility of PyTorch Edge folks?

One concern I have is if an unrelated change in torchtune breaks exportability CI, is it our responsibility to fix this? Is there any way to separate exportability CI from our core CI?

@Jack-Khuu
Copy link
Contributor Author

Thanks for checking in, we've been talking with @ebsmothers and the design is in the internal Diff comments, so that it doesn't pop up in OSS. I can add an abridged version to the PR as well

From what I gathered, the CI will live in torchtune and code quality/updating the CI for _export is largely the responsibility of PyTorch Edge folks? One concern I have is if an unrelated change in torchtune breaks exportability CI, is it our responsibility to fix this? Is there any way to separate exportability CI from our core CI?

Correct, PyTorch Edge will own and update the code and CI for these modules. If tune has changes that breaks exportability, Edge will champion the fix (the CI for this will be non-blocking for torchtune)

@RdoubleA
Copy link
Collaborator

Cool, thanks for the additional context. I can stamp this once the PR summary is updated.

@Jack-Khuu
Copy link
Contributor Author

Added some more detail @RdoubleA

Copy link
Contributor

@ebsmothers ebsmothers left a comment

Choose a reason for hiding this comment

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

Thanks all for weighing in here. Discussed with most folks in this thread offline already so I think we should be good to go on this PR

@ebsmothers ebsmothers merged commit bca5899 into pytorch:main Nov 15, 2024
19 checks passed
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. fb-exported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants