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.

Sign up for GitHub

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

Jump to bottom

[export] Move attrs to properties and add BC decorator #106170

Closed
wants to merge 1 commit into from
Closed

[export] Move attrs to properties and add BC decorator #106170

wants to merge 1 commit into from

Conversation

angelayi
Copy link
Contributor

@SherlockNoMad mentioned that it's not bc safe to directly access these attributes, so I moved them to @Property fields, and added a @compatibility decorator. For now I just set it to True for graph_module/graph.

@pytorch-bot
Copy link

pytorch-bot bot commented Jul 27, 2023 •
edited

🔗 Helpful Links

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

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

⏳ 1 Pending, 1 Unrelated Failure

As of commit 80c560a:

UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:

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

@angelayi angelayi changed the title [export] Move attrs to properties and add bc [export] Move attrs to properties and add BC decorator Jul 27, 2023
@facebook-github-bot
Copy link
Contributor

@angelayi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@angelayi @facebook-github-bot
[export] Move attrs to properties and add BC decorator (#106170)
80c560a 
Summary:
SherlockNoMad mentioned that it's not bc safe to directly access these attributes, so I moved them to property fields, and added a `compatibility` decorator. For now I just set it to True for graph_module/graph.

Pull Request resolved: #106170

Reviewed By: SherlockNoMad

Differential Revision: D47855081

Pulled By: angelayi

fbshipit-source-id: f17d2bc4cf6d3a754795dc731bb9460cd836a7c2
@facebook-github-bot
Copy link
Contributor

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

@angelayi
Copy link
Contributor Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 31, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@zhxchen17
Copy link
Contributor

@angelayi sorry I'm late, want to ask why graph_module is mutable? I think it will be a bit broken to do so.

@angelayi
Copy link
Contributor Author

angelayi commented Aug 2, 2023

@zhxchen17 For cases where just the graph module is transformed, I thought it might be easier to just do ep.graph_module = new_graph_module

@zhxchen17
Copy link
Contributor

@zhxchen17 For cases where just the graph module is transformed, I thought it might be easier to just do ep.graph_module = new_graph_module

we shouldn't support ep.graph_module = new_graph_module to end users right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SherlockNoMad SherlockNoMad SherlockNoMad approved these changes

@gmagogsfm gmagogsfm Awaiting requested review from gmagogsfm gmagogsfm is a code owner

@zhxchen17 zhxchen17 Awaiting requested review from zhxchen17 zhxchen17 is a code owner

@avikchaudhuri avikchaudhuri Awaiting requested review from avikchaudhuri avikchaudhuri is a code owner

@ydwu4 ydwu4 Awaiting requested review from ydwu4

@tugsbayasgalan tugsbayasgalan Awaiting requested review from tugsbayasgalan tugsbayasgalan is a code owner

Assignees
No one assigned
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: export
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@angelayi @facebook-github-bot @pytorchmergebot @zhxchen17 @SherlockNoMad