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

[Feature] Re-enable cache for specs #2730

Merged
merged 12 commits into from
Feb 3, 2025
Merged

Conversation

vmoens
Copy link
Contributor

@vmoens vmoens commented Jan 29, 2025

[ghstack-poisoned]
Copy link

pytorch-bot bot commented Jan 29, 2025

🔗 Helpful Links

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

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

❌ 7 New Failures, 1 Cancelled Job, 7 Pending, 5 Unrelated Failures

As of commit 2d84e84 with merge base cd4f359 (image):

NEW FAILURES - The following jobs have failed:

CANCELLED JOB - The following job was cancelled. Please retry:

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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

vmoens added a commit that referenced this pull request Jan 29, 2025
ghstack-source-id: 6a005e3e6e5a16a7d17a7b6977709fff5d43d4d0
Pull Request resolved: #2730
@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 Jan 29, 2025
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Jan 30, 2025
ghstack-source-id: f431f8666b94cd3b15904d4c89f2077b7a39c5bd
Pull Request resolved: #2730
@vmoens
Copy link
Contributor Author

vmoens commented Jan 30, 2025

This is a good clean-up PR:

  • Now specs are locked recursively if the env is locked. That was always the intention but not enforced, which is dangerous because without that your cached keys can be out of sync.

  • To change the specs you must unlock the env, which clears the cache.

  • The lock is similar to tensordict's and uses weakrefs to sub-specs to build a graph. You cannot unlock a node in the middle of the graph.

  • The locking occurs in the metaclass, which skips the super().__init__ and means that you can pass that kwarg even if it's not in your __init__.

  • I also made it possible to just pass a regular spec to the observation_spec in which case we assume it's a "observation" entry.

@vmoens vmoens added the performance Performance issue or suggestion for improvement label Jan 30, 2025
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Jan 30, 2025
ghstack-source-id: c05d3e4e4860c81a8e943c1152a82597aa9e9b35
Pull Request resolved: #2730
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Jan 30, 2025
ghstack-source-id: 3f66cd395a2b0088055cff8817a0ea2ba0205131
Pull Request resolved: #2730
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Jan 30, 2025
ghstack-source-id: 79e91ae59c4b26bbf151dfe828445339294d7e41
Pull Request resolved: #2730
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Jan 30, 2025
ghstack-source-id: ba96d69ccc774e7f421f3a14617c961824a5a040
Pull Request resolved: #2730
vmoens added a commit that referenced this pull request Jan 30, 2025
ghstack-source-id: ba96d69ccc774e7f421f3a14617c961824a5a040
Pull Request resolved: #2730
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Jan 31, 2025
ghstack-source-id: 6e503c1813724b2feb456122e22322ca83f14998
Pull Request resolved: #2730
@vmoens vmoens added the enhancement New feature or request label Jan 31, 2025
vmoens added a commit that referenced this pull request Jan 31, 2025
ghstack-source-id: 6e503c1813724b2feb456122e22322ca83f14998
Pull Request resolved: #2730
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Jan 31, 2025
ghstack-source-id: 883437617f6892f22c5d8b2746a6ee9348cde513
Pull Request resolved: #2730
@vmoens vmoens added the Environments Adds or modifies an environment wrapper label Feb 3, 2025
@vmoens vmoens added the Data Data-related PR, will launch data-related jobs label Feb 3, 2025
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Feb 3, 2025
ghstack-source-id: 04fcc45df12cf5cfe42966c602fb1281755bce9f
Pull Request resolved: #2730
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Feb 3, 2025
ghstack-source-id: 7d78c84c5bd23cf7c1ce6dc8db906c7f78a03b00
Pull Request resolved: #2730
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Feb 3, 2025
ghstack-source-id: 8acf690d89101008ac8bb2ae6b412379dc28f2e5
Pull Request resolved: #2730
[ghstack-poisoned]
vmoens added a commit that referenced this pull request Feb 3, 2025
ghstack-source-id: 797132312bfd9749f8926a2dd0b03eff65b8f51c
Pull Request resolved: #2730
@vmoens vmoens merged commit 2d84e84 into gh/vmoens/83/base Feb 3, 2025
59 of 75 checks passed
vmoens added a commit that referenced this pull request Feb 3, 2025
ghstack-source-id: 797132312bfd9749f8926a2dd0b03eff65b8f51c
Pull Request resolved: #2730
@vmoens vmoens deleted the gh/vmoens/83/head branch February 3, 2025 12:42
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. Data Data-related PR, will launch data-related jobs enhancement New feature or request Environments Adds or modifies an environment wrapper performance Performance issue or suggestion for improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants