read autograd templates and yaml files from resources #74672
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
Contributor
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit a1e378b (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
This was referenced Mar 24, 2022
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
dagitses
pushed a commit
that referenced
this pull request
Mar 24, 2022
Pull Request resolved: #74672 This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. ghstack-source-id: 152105009 Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)!
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
dagitses
pushed a commit
that referenced
this pull request
Mar 24, 2022
Pull Request resolved: #74672 This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. ghstack-source-id: 152105009 Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)!
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
dagitses
pushed a commit
that referenced
this pull request
Mar 24, 2022
Pull Request resolved: #74672 This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. ghstack-source-id: 152105009 Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)!
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
dagitses
pushed a commit
that referenced
this pull request
Mar 24, 2022
Pull Request resolved: #74672 This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. ghstack-source-id: 152105009 Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)!
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
dagitses
pushed a commit
that referenced
this pull request
Mar 25, 2022
Pull Request resolved: #74672 This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. ghstack-source-id: 152105009 Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)!
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
added 8 commits
March 28, 2022 12:16
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
albanD
reviewed
Apr 4, 2022
|
|
||
| # Note that even though this is deprecated, a certain Meta internal | ||
| # build is not compatible with importlib.resources file listing. | ||
| import pkg_resources |
Collaborator
There was a problem hiding this comment.
This import is very expensive #71902 can we do without it whenever possible?
Collaborator
Author
There was a problem hiding this comment.
I think that argument makes sense for the library we produce, but I don't think it makes much sense for our code generation. Does anything import this other than the build?
Also, this import isn't that slow on my crummy personal laptop:
$ hyperfine --warmup=2 $'python -c \'import pkg_resources\'' $'python -c \'from pkg_resources import packaging\''
Benchmark 1: python -c 'import pkg_resources'
Time (mean ± σ): 167.8 ms ± 25.1 ms [User: 139.0 ms, System: 14.0 ms]
Range (min … max): 127.7 ms … 217.0 ms 18 runs
Benchmark 2: python -c 'from pkg_resources import packaging'
Time (mean ± σ): 148.5 ms ± 24.4 ms [User: 127.9 ms, System: 13.8 ms]
Range (min … max): 117.6 ms … 186.8 ms 18 runs
Summary
'python -c 'from pkg_resources import packaging'' ran
1.13 ± 0.25 times faster than 'python -c 'import pkg_resources''
Collaborator
There was a problem hiding this comment.
Also, this import isn't that slow on my crummy personal laptop
IIRC it depends a lot of the software stack and configuration. It is not universally slow.
I agree it is not a hard blocker but our codegen takes ~10/20s to run. So if that takes 1s to import, it is a significant slowdown.
That's why I was asking if there is a way to have a simple guard to not use this when possible?
Collaborator
Author
There was a problem hiding this comment.
It's definitely possible. The downside would be that we would have to execute different code paths in different build systems. So long as something behaves the same as our internal systems, it's not a dealbreaker to me.
I wonder if so many other things are slower under a configuration that causes this import to take 1s to import that this 1s ends up not being that significant.
Collaborator
Author
There was a problem hiding this comment.
So, pkg_resources is used unconditionally by main. Moving the import lower would help if anything were to import this as a module. git grep import.*generate_code shows no results as of today.
Contributor
There was a problem hiding this comment.
To be frank, I don't like the idea myself. Can we workaround it somehow (working on a similar change for gen.pyi). Can't we just use filegroups and add those files as package sources?
Collaborator
Author
There was a problem hiding this comment.
Will that work the same way in Bazel and Buck? If you look at the internal diff corresponding to this, I think it was done somewhat like you suggest but that ended up needing different code paths between Buck and Bazel.
What are your objections specifically to this?
| # options.selected_op_list | ||
| operator_selector=get_selector(options.selected_op_list_path, options.operators_yaml_path), | ||
| ) | ||
| with tempfile.TemporaryDirectory() as autograd_dir_str: |
Collaborator
There was a problem hiding this comment.
Could you give some details on why we need to move all these files around?
Also where are these things used?
Collaborator
Author
There was a problem hiding this comment.
So the only reason I moved them around was to avoid modifying more of the code downstream, i.e. so downstream can just treat this like a regular directory unconditionally.
I could instead modify the bowels to read from pkg_resources instead of moving them to a temporary directory. What do you think?
If you would rather me keep it this way, I'm happy to leave a comment explaining why we're doing this.
I do want to avoid having multiple ways of accessing these templates. I'd like to have a consistent way of getting them for cmake, buck, and bazel.
Collaborator
There was a problem hiding this comment.
I agree that moving them around to avoid having weird code to access them is good.
A slightly more detailed comment here would be great for the next person that will have to touch this code!
Collaborator
Author
There was a problem hiding this comment.
Oops. I did have a more detailed comment elsewhere but it got orphaned when I moved the code. How does this look now?
Collaborator
There was a problem hiding this comment.
I don't see any extra comment in this PR?
Collaborator
Author
There was a problem hiding this comment.
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
added 5 commits
April 6, 2022 02:29
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
Closed
added 5 commits
April 6, 2022 13:21
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
This will allow us to be consistent between internal and OSS because we can use resources/data in Buck and Bazel respectively. Differential Revision: [D35112243](https://our.internmc.facebook.com/intern/diff/D35112243/) **NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D35112243/)! [ghstack-poisoned]
albanD
approved these changes
Apr 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
This will allow us to be consistent between internal and OSS because
we can use resources/data in Buck and Bazel respectively.
Differential Revision: D35112243
NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!